Closed
Bug 1276354
Opened 9 years ago
Closed 9 years ago
Hovering over a "downstream alert" should pop up a tooltip with title
Categories
(Tree Management :: Perfherder, defect)
Tree Management
Perfherder
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: wlach, Assigned: crosscent)
Details
Attachments
(1 file)
Recently Arpan added support for showing a list of "downstream alerts" to an alert summary in bug 1247028. You can see an example of this here:
https://treeherder.mozilla.org/perf.html#/alerts?id=1328
It would be nice if hovering over these numbers actually gave their title in a tooltip. We already have code in perfherder to get the title of an alert summary object once we've downloaded it:
https://github.com/mozilla/treeherder/blob/d4217d538279b86866aef8cc5a5e9322d74400b9/ui/js/models/perf/alerts.js#L95
So this bug would involve several things, I think:
1. Adding a method to the alerts model to get a title for an alert summary given an id.
2. Adding a tooltip (http://angular-ui.github.io/bootstrap/#/tooltip) to the alertsctrl, and filling its content using the method above. Ideally we would also show a spinner while we were downloading this information (do a git grep for fa-spin for other examples of this).
If this approach works well we could probably also do this elsewhere in the UI (such as for the individual alerts), but let's start here. :)
I am having trouble with the first task already :(
What I am thinking is:
1. Create a method for the prototype of AlertSummary
2. Access the correct alert inside AlertSummary.
3. return the title
What I think you mean:
1. Create a method in the factory
2. Call DRF and create a new AlertSummary object
3. and return the title
But I can't call the method defined in the factory within itself, or in the template, and copying getAlertSummary seems too inefficient.
Thanks for all your help with these AngularJS related problems!
(In reply to William Lachance (:wlach) from comment #0)
> Recently Arpan added support for showing a list of "downstream alerts" to an
> alert summary in bug 1247028. You can see an example of this here:
>
> https://treeherder.mozilla.org/perf.html#/alerts?id=1328
>
> It would be nice if hovering over these numbers actually gave their title in
> a tooltip. We already have code in perfherder to get the title of an alert
> summary object once we've downloaded it:
>
> https://github.com/mozilla/treeherder/blob/
> d4217d538279b86866aef8cc5a5e9322d74400b9/ui/js/models/perf/alerts.js#L95
>
> So this bug would involve several things, I think:
>
> 1. Adding a method to the alerts model to get a title for an alert summary
> given an id.
> 2. Adding a tooltip (http://angular-ui.github.io/bootstrap/#/tooltip) to the
> alertsctrl, and filling its content using the method above. Ideally we would
> also show a spinner while we were downloading this information (do a git
> grep for fa-spin for other examples of this).
>
> If this approach works well we could probably also do this elsewhere in the
> UI (such as for the individual alerts), but let's start here. :)
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to crosscent from comment #1)
> What I am thinking is:
>
> 1. Create a method for the prototype of AlertSummary
> 2. Access the correct alert inside AlertSummary.
> 3. return the title
>
> What I think you mean:
>
> 1. Create a method in the factory
> 2. Call DRF and create a new AlertSummary object
> 3. and return the title
>
> But I can't call the method defined in the factory within itself, or in the
> template, and copying getAlertSummary seems too inefficient.
Yup, you have good intuitions on this. In fact we can get around the problem you mention by defining a getAlertSummary before the return statement, like this:
```js
function _getAlertSummary(id) {
..
}
return {
getAlertSummary: _getAlertSummary,
getAlertTitle: function(id) {
return _getAlertSummary(id).then(function(alertSummary) {
..
}
},
...
```
Hope that makes sense!
Comment 3•9 years ago
|
||
Attachment #8758792 -
Flags: review?(wlachance)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8758792 [details] [review]
[treeherder] crosscent:bug1276354 > mozilla:master
Just tried this and it works great! Just two fix ups required and we should be good to land.
Attachment #8758792 -
Flags: review?(wlachance) → review-
(In reply to William Lachance (:wlach) from comment #4)
> Just tried this and it works great! Just two fix ups required and we should
> be good to land.
Changes have been made!
Attachment #8758792 -
Flags: review- → review?(wlachance)
Comment 6•9 years ago
|
||
Commit pushed to master at https://github.com/mozilla/treeherder
https://github.com/mozilla/treeherder/commit/c3085283a5a0d33076591930e29f99954bac8ee7
Bug 1276354 - Display downstream alert title on hover
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8758792 [details] [review]
[treeherder] crosscent:bug1276354 > mozilla:master
There were some minor problems with the spinner, which I corrected before pushing. Otherwise this looks great! Thanks so much.
Attachment #8758792 -
Flags: review?(wlachance) → review+
Reporter | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(In reply to William Lachance (:wlach) from comment #7)
> There were some minor problems with the spinner, which I corrected before
> pushing. Otherwise this looks great! Thanks so much.
Thanks for fixing that Will!
You need to log in
before you can comment on or make changes to this bug.
Description
•