Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(alerts): alerts button on top of insight #26116

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

anirudhpillai
Copy link
Contributor

Problem

Hard to discover that you can set alerts on insight

Changes

Added a button to manage alerts right on top of the insight so users can easily access it. Can also show count of alerts set in the button but first wanted to check people are happy with the UX change.

Note: I've added it as part of the 'caption' of the page header. Thought this was cleaner as the user activity indicator doesn't need the full space.

image

Does this work well for both Cloud and self-hosted?

N/A

How did you test this code?

Tested locally

@anirudhpillai anirudhpillai requested a review from a team November 11, 2024 16:04
Copy link
Contributor

github-actions bot commented Nov 11, 2024

Size Change: 0 B

Total Size: 1.15 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.15 MB

compressed-size-action

Copy link
Contributor

@thmsobrmlr thmsobrmlr left a comment

Choose a reason for hiding this comment

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

Can also show count of alerts set in the button ...

That would be great! Can of course come later as well.

... but first wanted to check people are happy with the UX change.

Fine for me to go in here - should probably be size "small" then to fit in with the other buttons at the top. Another suggestion would be to put it next to the "detailled results" table next to the export button. After all exports and alerts are similar in that they get data out.

A bit of nit-picking:

  • Going to "edit mode" makes the button disappear, which is strange. The button therefore also disappears when the alerts modal is open. Maybe you want to check if this is a persisted insight (hasDashboardItemId)?
  • The text for the disabled reason says "Insights are only available for trends without breakdowns". Shouldn't it be "Alerts are only available for trends without breakdowns."
  • Tests are failing somehow.

@anirudhpillai
Copy link
Contributor Author

Thanks Thomas! Have moved the button to the top header. I wanted it there originally but was hesitant in case it uses it up too much space, though seems good now.
Also added the icon to indicate number of alerts setup
image

Thanks for checking, have also fixed the warning text
image

Have also updated the tests, was waiting to update the reference to the button after the UX is approved :)

Just regarding Going to "edit mode" makes the button disappear. I had done this intentionally as users can change the insight while editing so that alerts aren't supported anymore. Now it's also consistent with the other buttons in the top bar which also get hidden in edit mode
image

Copy link
Contributor

@thmsobrmlr thmsobrmlr left a comment

Choose a reason for hiding this comment

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

Nice work! Two things, but they’re just observations—no need to take any action on them.

  1. We could remove the "Add to..." text from notebooks and dashboards as well. Would be more uniform and take up less space:
Screenshot 2024-11-13 at 10 09 48
  1. When I add an alert and then change the insight type to e.g. funnel, the button looks like the following. I don't know how the alert will work now and can't edit it:
Screenshot 2024-11-13 at 10 10 18

@anirudhpillai anirudhpillai merged commit d5ce604 into master Nov 13, 2024
96 checks passed
@anirudhpillai anirudhpillai deleted the feat/alerts-button-on-insight branch November 13, 2024 09:15
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.

2 participants