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

[Alerting] Add popover for Elasticsearch query rule type #135968

Merged
merged 14 commits into from
Jul 15, 2022

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Jul 8, 2022

Summary

Relates to #134763

That PR included edits to the UI text for the Elasticsearch query rule type and removed a tooltip associated with the query threshold fields.

This PR:

Screenshots

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lcawl lcawl added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) ui-copy Review of UI copy with docs team is recommended v8.4.0 labels Jul 8, 2022
@lcawl lcawl removed the release_note:skip Skip the PR/issue when compiling release notes label Jul 8, 2022
@lcawl lcawl requested review from gchaps, mdefazio and a team July 8, 2022 20:12
@lcawl lcawl marked this pull request as ready for review July 8, 2022 20:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@lcawl lcawl added the release_note:skip Skip the PR/issue when compiling release notes label Jul 8, 2022
Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

This is a nice start. Here are a few comments.

Placement

Can this pop-up cover just the greyed out background and not the Create rule form? It would nice to see the rest of the form for context.

We should look at other help pop-ups and use the same width.

Formatting

  • Larger font size for title

  • Re: the steps, @KOTungseth reminded me that when we first started implementing in-product assistance, we talked about leaving step-by-step content in the product docs, and reference content in the product. So maybe this (with or without the bullets):

  • The threshold value is a comparison operator, for exampel, IS ABOVE 1000. ...

  • The time is how far back in time to search. To avoid gaps...

Text

Overall, I think we can cut down the text a bit.

@mdefazio
Copy link
Contributor

mdefazio commented Jul 8, 2022

A few thoughts:

I think we'll at least want to put a max-width on the popover panel to account for wide screens...

image

What if we use a tour component instead? It has a maxW prop and we can also break apart the steps based on the input (the multi doc callout might prove tricky though).

I'm wondering if this helps us a) avoid taking over the screen and b) is readable in the context of the input (depending on the screen size and how far I've scrolled when I click the help, it may hide the inputs).

image

Here's a rough breakdown of what I was imagining.
Whimsical link

image

Granted, I think this does beg the question of whether this should go through the whole form...

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Just have the one comment on the width of the popover.

I think we can explore some other options on incorporating this copy within the form. But this certainly is an improvement.

@lcawl
Copy link
Contributor Author

lcawl commented Jul 14, 2022

Can this pop-up cover just the greyed out background and not the Create rule form? It would nice to see the rest of the form for context.

I didn't see how to make that happen, but I did change the anchor position so it's less likely to cover the pertinent fields.

  • Larger font size for title

I was unable to force the popover title and the callout title to match, so I've removed the callout title.

  • Re: the steps, @KOTungseth reminded me that when we first started implementing in-product assistance, we talked about leaving step-by-step content in the product docs...

Sure, I've changed it back to paragraphs.

Overall, I think we can cut down the text a bit.

I've reduced the callout to a single line. I think we could likely also drop the "For example..." phrase, but if you see other areas to cut, let me know!

…ommon_expressions/threshold_help_popover.tsx
Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

LGTM!

@gchaps
Copy link
Contributor

gchaps commented Jul 14, 2022

Looking good.

I think we should be more direct in the second paragraph:

To avoid gaps in detection, set this value greater than or equal to the value you chose for the Check every field.

Hopefully, the pop-up doesn't cover the "Check every" field.

I like the blue box at the bottom and without the title. Is there a way we can indicate this is a note? Maybe an icon and the word note:

x Note: If the time window ....

Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

LGTM

@lcawl lcawl enabled auto-merge (squash) July 14, 2022 23:08
@lcawl
Copy link
Contributor Author

lcawl commented Jul 14, 2022

I think we should be more direct in the second paragraph:

Thanks! I made that change in c6592f6

Hopefully, the pop-up doesn't cover the "Check every" field.

Unfortunately the "check every" field is right near the top of that UI so folks have likely scrolled too far to see it anymore. Hopefully it's still fresh in their memory.

I like the blue box at the bottom and without the title. Is there a way we can indicate this is a note? Maybe an icon and the word note:

Since it's a single sentence in that callout now, I was able to get the icon to re-appear (it disappeared when I removed the title) by turning the sentence into the title: 1718236. I've refreshed the initial screenshot with the result.

@lcawl lcawl merged commit 2679970 into elastic:main Jul 15, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
stackAlerts 136 137 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
stackAlerts 209.6KB 211.8KB +2.1KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@lcawl lcawl deleted the rule-popover branch July 18, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) ui-copy Review of UI copy with docs team is recommended v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants