1206173 - Have a loading symbol when changing the date range in Perfherder

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[Vaibhav (:vaibhav1994)]

When changing a date range (eg: last 7 days -> last 30 days) in perfherder while looking at a graph, perfherder sometimes takes a good amount of time to load it. I don't know if perfherder is actually loading it or there is some kind of problem. It would be good to have a loading symbol like in graph-server, probably beside the drop down menu, to know that perfherder is doing background work for it.

Comment 1

[William Lachance (:wlach)]

Yeah we could do this. Note that the current series loading code in perfherder is rather inefficient (it loads series one at a time rather than all at once) which might improve things by itself. In general, I'd rather make things fast where possible than add loading indicators. :)

Comment 2

[Vaibhav (:vaibhav1994)]

True, I would like to see improvements for showing results in perfherder! :jmaher told me that there was patch landed to make this faster, lets see if makes the experience better, and then we might not have to add this symbol anymore!

Comment 3

[Vibhor Sehgal]

Hi,

I would like to solve this bug. I have implemented the basic part of the bug. A rotating Loading symbol just below the drop down menu . Looks like this : 
http://s32.postimg.org/snvt2ccwl/loading.jpg

It's actually a gif which keeps rotating. Can you suggest any more improvements or changes to this?
Thanks.
Vibhor Sehgal

Comment 4

[William Lachance (:wlach)]

Hi Vibhor! Sorry I missed this earlier. That looks like a great start. I'd prefer a style more like what we have in the job details panel in treeherder:

https://github.com/mozilla/treeherder/blob/master/ui/css/treeherder-info-panel.css#L126
https://github.com/mozilla/treeherder/blob/master/ui/plugins/similar_jobs/main.html

It probably makes sense to add the CSS to ui/css/treeherder-global.css and import that into ui/perf.html. There might be a bit of extra work involved there to make things look right.

If you're still interested in this bug let me know and I'll assign it to you. :)

Comment 5

[William Lachance (:wlach)]

It looks like vibhor's not around. I'll give it to shruti

Comment 6

[William Lachance (:wlach)]

For extra bonus points, we could make this an angular component so we don't duplicate this boilerplate over and over:

```html
    <div ng-if="tabs.failureSummary.is_loading" class="overlay">
        <div>
            <span class="fa fa-spinner fa-pulse th-spinner-lg"></span>
        </div>
    </div>
```

If you take that approach, I would put the javascript in `treeherder/ui/js/components/loading.js`

Comment 7

[William Lachance (:wlach)]

Shruti doesn't have time for this atm, leaving it for someone else to pick up.

Comment 8

[Shruti Jasoria [:ShrutiJ]]

wlach, can I take up this bug?

Comment 9

[William Lachance (:wlach)]

(In reply to Shruti Jasoria [:ShrutiJ] from comment #8)
> wlach, can I take up this bug?

Of course, feel free to assign it to yourself. You have the permissions! :)

Comment 10

[GitHub Autolander Bot]

Attachment: [treeherder] SJasoria:bug1206173 > mozilla:master

Comment 11

[Shruti Jasoria [:ShrutiJ]]

Comment on attachment 8819773 [details] [review]
[treeherder] SJasoria:bug1206173 > mozilla:master

Hi, I have made an angular component to implement the html script in comment 6. Let me know if anymore changes have to be made.

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8819773 [details] [review]
[treeherder] SJasoria:bug1206173 > mozilla:master

This looks pretty good, but could you make it so that the loading panel shows whenever something is loading? This includes the following cases:

1. When coming to the graphs page from a link.
2. When adding a series to the page.

Thanks!

Comment 13

[Shruti Jasoria [:ShrutiJ]]

Comment on attachment 8819773 [details] [review]
[treeherder] SJasoria:bug1206173 > mozilla:master

I have made a few more changes to include the cases which you've mentioned in comment 12. I hope it's good now.

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8819773 [details] [review]
[treeherder] SJasoria:bug1206173 > mozilla:master

Hi Shruti, the code looks good now, however the CSS/HTML needs a bit of tweaking to get the right effect. Could you take another look?

Comment 15

[Shruti Jasoria [:ShrutiJ]]

Comment on attachment 8819773 [details] [review]
[treeherder] SJasoria:bug1206173 > mozilla:master

I have changed perf.css a bit to get the required loading effect. Let me know if anymore changes have to be made.

Comment 16

[William Lachance (:wlach)]

Comment on attachment 8819773 [details] [review]
[treeherder] SJasoria:bug1206173 > mozilla:master

This is looking really good! Just a few more changes requested and then we can land.

Comment 17

[Shruti Jasoria [:ShrutiJ]]

Comment on attachment 8819773 [details] [review]
[treeherder] SJasoria:bug1206173 > mozilla:master

I have made the changes you had asked for in the PR. I hope it's good to land now.

Comment 18

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/76286eb5bdf2ae77767c45052d6ee7e0115f7138
Bug 1206173 - Have a loading symbol when changing the date range in Perfherder (#2031)

Comment 19

[William Lachance (:wlach)]

Comment on attachment 8819773 [details] [review]
[treeherder] SJasoria:bug1206173 > mozilla:master

Yup, everything looks good now! Thanks so much.