Skip to content

Collapse alert groups when initially viewing ui#1876

Merged
stuartnelson3 merged 11 commits into
masterfrom
stn/collapse-alert-groups
May 13, 2019
Merged

Collapse alert groups when initially viewing ui#1876
stuartnelson3 merged 11 commits into
masterfrom
stn/collapse-alert-groups

Conversation

@stuartnelson3
Copy link
Copy Markdown
Contributor

image

helps with frontend rendering, and (in my opinion) makes it more usable. clicking the + expands the group, much like showing/hiding annotations for individual alerts.

requested in #911

@stuartnelson3 stuartnelson3 force-pushed the stn/collapse-alert-groups branch from 39bcfd5 to 03f1b63 Compare May 3, 2019 13:53
Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
@stuartnelson3 stuartnelson3 force-pushed the stn/collapse-alert-groups branch from 03f1b63 to b4fb8a2 Compare May 3, 2019 14:15
@w0rm
Copy link
Copy Markdown
Member

w0rm commented May 3, 2019

As far as I understand this would only allow to expand one group. Can we allow expanding multiple groups? Can we also have a button to expand all groups?

If there is a single group, can we always expand it?

If we add a button to expand all groups, can we keep this preference in the localStorage like the username?

@stuartnelson3
Copy link
Copy Markdown
Contributor Author

As far as I understand this would only allow to expand one group. Can we allow expanding multiple groups?

We definitely can. I thought about this but did the simple thing first. Also, since we only allow a single alert's annotation information to be open at a time, I thought I could do that as well.

I'm guessing, for implementation, we would store a List of Labels and compare against that.

Can we also have a button to expand all groups?

We definitely could. I'm wondering how often it would be used. But, if there are users/alertmanager installations running that have relatively few alerts/alert groups, maybe it would make sense to provide this.

Maybe you could give some suggestions on how to show this, and where to place it (I have very little UX skill..)

If we add a button to expand all groups, can we keep this preference in the localStorage like the username?

+1

If there is a single group, can we always expand it?

Makes sense

@w0rm
Copy link
Copy Markdown
Member

w0rm commented May 3, 2019

@stuartnelson3 let’s chat about this on Monday

More ideas:
Would it be cool to collapse groups with too many alerts and auto-expand groups with a few?
Shall we show the number of alerts in a group?

@beorn7
Copy link
Copy Markdown
Contributor

beorn7 commented May 6, 2019

Thanks for the PR and the discussion. Dynamically autoexpanding if a group has very few alerts and/or there are very few groups altogether would be awesome (cf. how GitHub renders notifications https://github.com/notifications , they even have partial expansion there, perhaps that would be interesting here as well, to see an example alert").

Would be great to see a number next to unexpanded groups ("44 alerts") or partially expanded groups ("41 more alerts, see more...")

@stuartnelson3
Copy link
Copy Markdown
Contributor Author

stuartnelson3 commented May 7, 2019

My plans for this PR:

  • for a single group, expand it always
  • support expanding multiple groups
  • add a button to toggle expand/collapse all (user setting stored in localStorage)
  • add alert count behind group labels

@beorn7
Copy link
Copy Markdown
Contributor

beorn7 commented May 7, 2019

And of course, this PR doesn't have to solve everything. It's already a great start.

We can simplify the view flow by parsing the list
of alerts for custom grouping as soon as they are
returned to the update function.

Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
Can't think of a reason to hide the alerts if
there's only a single group.

Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
@stuartnelson3 stuartnelson3 force-pushed the stn/collapse-alert-groups branch from 0686e62 to e8d1840 Compare May 7, 2019 14:08
@stuartnelson3
Copy link
Copy Markdown
Contributor Author

@w0rm could you help a bit with the styling? in particular, the alert count seems to be on the "baseline" and should probably be bumped up higher, and the "expand all" button doesn't quite seem to be lined up correctly.

image

Comment thread ui/app/index.html
production: true,
defaultCreator: localStorage.getItem('defaultCreator')
defaultCreator: localStorage.getItem('defaultCreator'),
groupExpandAll: JSON.parse(localStorage.getItem('groupExpandAll'))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a lot of anger and confusion, I did some reading and figured out that booleans cannot be stored into local storage. Is this an ok way to get the boolean back out? Not sure if there's a convention around it.

( Loading, groupBar )

Failure e ->
( Failure e, groupBar )
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sorting the alerts into alert groups immediately when they're returned from the API. I wasn't sure how to have a pattern match Initial, Loading, and Failure e, because they're technically of type ApiData GettableAlerts. If there's a way to compact this, that would be awesome.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine.

@w0rm w0rm force-pushed the stn/collapse-alert-groups branch 3 times, most recently from dea2dbe to c933585 Compare May 11, 2019 18:26
Andrey Kuzmin added 3 commits May 11, 2019 20:28
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
@w0rm w0rm force-pushed the stn/collapse-alert-groups branch from c933585 to 0f1df58 Compare May 11, 2019 18:28
@w0rm
Copy link
Copy Markdown
Member

w0rm commented May 11, 2019

@stuartnelson3 I adjusted the styling:

Screenshot 2019-05-11 at 20 31 55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants