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.

Sign up for GitHub

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

Jump to bottom

Alerting: Expression card improvements #70395

Merged
merged 16 commits into from
Jun 27, 2023
Merged

Alerting: Expression card improvements #70395

merged 16 commits into from
Jun 27, 2023

Conversation

soniaAguilarPeiron
Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron commented Jun 20, 2023 •
edited
Loading

What is this feature?

This PR adds several changes to the expression section, providing a more streamlined and intuitive experience.

Why do we need this feature?

We introduce some improvements regarding ease of use and consistency.

Who is this feature for?

All users

Which issue(s) does this PR fix?:

Fixes #68194
Fixes #68195

Special notes for your reviewer:

improve-expressions.webm

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@soniaAguilarPeiron soniaAguilarPeiron added this to the 10.1.x milestone Jun 20, 2023
@soniaAguilarPeiron soniaAguilarPeiron self-assigned this Jun 20, 2023
@soniaAguilarPeiron soniaAguilarPeiron requested a review from a team as a code owner June 20, 2023 16:15
@soniaAguilarPeiron soniaAguilarPeiron requested a review from a team June 20, 2023 16:15
@soniaAguilarPeiron soniaAguilarPeiron marked this pull request as draft June 20, 2023 16:15
@soniaAguilarPeiron soniaAguilarPeiron marked this pull request as ready for review June 21, 2023 15:39
@gillesdemey
Copy link
Member

I think there are a few small UI issues to fix here (like the empty footer which looks a bit out-of-place) and I'm a tad concerned about the dual-column design which isn't very responsive for larger screens.

@soniaAguilarPeiron
Copy link
Member Author

soniaAguilarPeiron commented Jun 21, 2023 •
edited
Loading

what do you mean with the 'empty footer'? @gillesdemey

and I'm a tad concerned about the dual-column design which isn't very responsive for larger screens.

we should tag @dhalachliyski regarding this one

@gillesdemey
Copy link
Member

gillesdemey commented Jun 21, 2023 •
edited
Loading

What I was referring to was the footer for each expression, those are empty unless you've both set the expression as an alert condition and have previewed the query. It looks a bit odd and out-of-place visually so we might just want to omit it if both of those conditions aren't met :)

Screenshot 2023-06-21 at 18 01 39

@soniaAguilarPeiron
Copy link
Member Author

soniaAguilarPeiron commented Jun 21, 2023 •
edited
Loading

What I was referring to was the footer for each expression, those are empty unless you've both set the expression as an alert condition and have previewed the query. It looks a bit odd and out-of-place visually so we might just want to omit it if both of those conditions aren't met :)

Good catch! yes, I agree.

@gillesdemey gillesdemey mentioned this pull request Jun 22, 2023
Merged
3 tasks
@soniaAguilarPeiron soniaAguilarPeiron force-pushed the alerting/expression-card-improvements branch from b6907eb to 20f4db8 Compare June 23, 2023 12:02
@soniaAguilarPeiron soniaAguilarPeiron force-pushed the alerting/expression-card-improvements branch from a6c40bb to 6fa5b79 Compare June 23, 2023 13:13
@gillesdemey
Copy link
Member

gillesdemey commented Jun 23, 2023 •
edited
Loading

I think in terms of expression comprehension we've taken a step back here – some of the other improvements in this PR are great but the expression card layout isn't one of them.

Below some screenshots with multiple expressions (before / after) and some thoughts

Before

Screenshot 2023-06-23 at 15 25 37

  • Each "row" in the expression is aligned regardless of the height of the expression's editor
  • Multiple expressions can be fitted on the same row if my screen allows for it

After

Screenshot 2023-06-23 at 15 25 44

  • Series are no longer aligned, shifted by one row (presumably the lack of footer)
  • It only fits about 2 expressions on this size, about 4 prior

I'd love to cherry pick the improvements that make sense so we can merge the PR.

The dynamic width of the chart visualization is great, I love the new expression "picker" with the dropdown and the alert condition in the header makes sense (though it was in the footer so we could align time series).

gillesdemey
gillesdemey approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@gillesdemey gillesdemey left a comment

Choose a reason for hiding this comment

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

Great improvements, LGTM! :shipit:

@soniaAguilarPeiron soniaAguilarPeiron merged commit b0ac499 into main Jun 27, 2023
14 checks passed
@soniaAguilarPeiron soniaAguilarPeiron deleted the alerting/expression-card-improvements branch June 27, 2023 09:35
harisrozajac pushed a commit that referenced this pull request Jun 29, 2023
@soniaAguilarPeiron @gillesdemey @harisrozajac
Alerting: Expression card improvements (#70395)
0c6ea85 
* Show description for each expression type in the body and change widht depending on the type

* Move condition indicator to the header

* Make order of fields in expressions to be consistent for each expression type

* Add tooltip for expression type menu

* Update styles depending on the expression type

* Update styles and move add query button under queries

* Add NeedHelpInfo component

* Adress PR review comments

* Apply description updates from #70540

* Rename gelTypes to expressionTypes

* Update layout for expressions according to the real usecases

* Update footer to include series count in all expressions

---------

Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
harisrozajac pushed a commit that referenced this pull request Jun 30, 2023
@soniaAguilarPeiron @gillesdemey @harisrozajac
Alerting: Expression card improvements (#70395)
2ebe69c 
* Show description for each expression type in the body and change widht depending on the type

* Move condition indicator to the header

* Make order of fields in expressions to be consistent for each expression type

* Add tooltip for expression type menu

* Update styles depending on the expression type

* Update styles and move add query button under queries

* Add NeedHelpInfo component

* Adress PR review comments

* Apply description updates from #70540

* Rename gelTypes to expressionTypes

* Update layout for expressions according to the real usecases

* Update footer to include series count in all expressions

---------

Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
@chenrui333 chenrui333 mentioned this pull request Aug 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gillesdemey gillesdemey gillesdemey approved these changes

@brendamuir brendamuir brendamuir approved these changes

@dhalachliyski dhalachliyski Awaiting requested review from dhalachliyski

@konrad147 konrad147 Awaiting requested review from konrad147

Assignees

@soniaAguilarPeiron soniaAguilarPeiron

Labels
add to changelog area/alerting Grafana Alerting area/frontend kata:alert-rule-creation no-backport Skip backport of PR
Projects
Alerting
Archived in project
Milestone
10.1.0
Development

Successfully merging this pull request may close these issues.

Make add expression button as a menu so we can select the type on it Expression card improvements
5 participants
@soniaAguilarPeiron @gillesdemey @konrad147 @brendamuir @ricky-undeadcoders