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

Add description and documentation link in alert flyout #81526

Merged
merged 10 commits into from
Nov 6, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Oct 22, 2020

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) Screen Shot 2020-11-05 at 9 37 05 AM

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 a documentationUrl property that can have one of the following values:

  • null
  • string
  • (docLinks: DocLinksStart) => string

@mikecote mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 labels Oct 22, 2020
@mikecote mikecote self-assigned this Oct 22, 2020
@mikecote mikecote changed the title Improve UX for finding documentation for each alert type Add documentationUrl and link to documentation in alert flyout Oct 28, 2020
@mikecote mikecote changed the title Add documentationUrl and link to documentation in alert flyout Add description and documentation link in alert flyout Oct 28, 2020
@mikecote mikecote marked this pull request as ready for review November 4, 2020 17:24
@mikecote mikecote requested review from a team as code owners November 4, 2020 17:24
@mikecote mikecote requested a review from a team November 4, 2020 17:24
@mikecote mikecote requested a review from a team as a code owner November 4, 2020 17:24
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@ymao1
Copy link
Contributor

ymao1 commented Nov 4, 2020

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

Example of tight spacing:
Screen Shot 2020-11-04 at 3 34 17 PM

@ymao1
Copy link
Contributor

ymao1 commented Nov 4, 2020

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.

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.

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

@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Nov 4, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

apm docs lgtm

@mikecote
Copy link
Contributor Author

mikecote commented Nov 5, 2020

@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 name field auto selected to trigger validation when moving away but I would have to dig into some old issues to make sure we don't bring back new issues. Maybe worth a UX issue and we can discuss there? (I'm happy to create the issue)

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Uptime changes LGTM !!

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

Infra LGTM

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ymao1
Copy link
Contributor

ymao1 commented Nov 5, 2020

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 name field auto selected to trigger validation when moving away but I would have to dig into some old issues to make sure we don't bring back new issues. Maybe worth a UX issue and we can discuss there? (I'm happy to create the issue)

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 :)

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! Thanks for making the edits.

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

@mikecote mikecote merged commit 1ecd12c into elastic:master Nov 6, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Nov 6, 2020
* 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
mikecote added a commit that referenced this pull request Nov 6, 2020
* 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
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
monitoring 965.0KB 966.0KB +1.0KB
triggersActionsUi 1.5MB 1.5MB +1.1KB
total +2.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
apm 48.2KB 48.8KB +584.0B
infra 173.2KB 173.7KB +502.0B
triggersActionsUi 132.5KB 132.7KB +196.0B
uptime 24.7KB 25.2KB +488.0B
total +1.7KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
_doc_count - 1 +1

History

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

@mikecote mikecote added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:enhancement Team:APM All issues that need APM UI Team support Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve UX for finding documentation for each alert type
10 participants