-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add description and documentation link in alert flyout #81526
Conversation
afdff90
to
0099766
Compare
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
Looks good! I noticed that in the mockup, there is another horizontal rule at the bottom of the "header" section of the alert type (containing the name, description & doc link). Do we want to add that in this PR? Otherwise, the rules section of the alert type is sometimes very tight against the doc link |
I think this is unrelated to this PR, but often when I open the flyout and click directly on the doc link, it triggers the form validation, which shows an error for the "Name" field, which makes all the form fields jump down to accommodate, and then I have to click the doc link again to actually open. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch on the spacing. I think we can also just add this right after the description, instead of forcing it to its own line.
- Change
EuiText
size to small for the description - Move documentation link inside the description with
EuiLink
Pinging @elastic/apm-ui (Team:apm) |
Pinging @elastic/uptime (Team:uptime) |
x-pack/plugins/uptime/public/lib/alert_types/monitor_status.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apm docs lgtm
@ymao1 good catch! I added the horizontal ruler and also applied the change @mdefazio proposed. Regarding the UX when clicking the link, I see exactly what you mean and it happens when the alert type is pre-selected when opening the flyout. I recall there was a reason why we made the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uptime changes LGTM !!
x-pack/plugins/monitoring/public/alerts/disk_usage_alert/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/monitoring/public/alerts/legacy_alert/legacy_alert.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/monitoring/public/alerts/memory_usage_alert/index.tsx
Outdated
Show resolved
Hide resolved
...ins/monitoring/public/alerts/missing_monitoring_data_alert/missing_monitoring_data_alert.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Infra LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I think it's a minor annoyance and maybe most people won't go straight to clicking on the Documentation link? I would be fine with waiting to see if any user complains :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for making the edits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add description and documentation URL in alert flyout * Add unit tests * Fix type check * Add horizontal rule * Design fixes * Fix uptime alert link * Fix uptime urls * Add anchor tag * Fix jest test failures * Fix monitoring links
* Add description and documentation URL in alert flyout * Add unit tests * Fix type check * Add horizontal rule * Design fixes * Fix uptime alert link * Fix uptime urls * Add anchor tag * Fix jest test failures * Fix monitoring links
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
Part of and resolves #75548.
In this PR, I'm adding a
documentationUrl
property to alert types. I've added what I believe is the correct url for each. I'm also displaying the link and description in the alert flyout.This is based on the mockup from #64077 (comment).
Screenshot (see middle)
Note to codeowners
I think I've found all the proper documentation links for each alert type. Please verify that they are accurate.
Note for docs
The screenshots for create / edit flyout will need to be updated because there is now a description and documentation link displayed.
From a technical perspective, the
AlertTypeModel
in the UI now has adocumentationUrl
property that can have one of the following values:null
string
(docLinks: DocLinksStart) => string