Skip to content

[Alerting UI] Wrapping text on create rule flyout#113305

Merged
ymao1 merged 4 commits intoelastic:masterfrom
ymao1:alerting/long-descriptions-bug
Oct 6, 2021
Merged

[Alerting UI] Wrapping text on create rule flyout#113305
ymao1 merged 4 commits intoelastic:masterfrom
ymao1:alerting/long-descriptions-bug

Conversation

@ymao1
Copy link
Contributor

@ymao1 ymao1 commented Sep 28, 2021

Resolves #109852

Summary

Added wrapText to EuiListGroupItem in order to wrap the text for long rule type descriptions

Screen Shot 2021-09-28 at 1 45 02 PM

@ymao1
Copy link
Contributor Author

ymao1 commented Oct 1, 2021

@elasticmachine merge upstream

@ymao1 ymao1 self-assigned this Oct 1, 2021
@ymao1 ymao1 added Feature:Alerting/RulesManagement Issues related to the Rules Management UX release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.16.0 v8.0.0 labels Oct 1, 2021
@ymao1 ymao1 marked this pull request as ready for review October 1, 2021 17:13
@ymao1 ymao1 requested a review from a team as a code owner October 1, 2021 17:13
@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 1, 2021

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

@ymao1 ymao1 changed the title Wrapping text on create rule flyout [Alerting UI] Wrapping text on create rule flyout Oct 1, 2021
@ymao1
Copy link
Contributor Author

ymao1 commented Oct 4, 2021

@elasticmachine merge upstream

YulNaumenko
YulNaumenko reviewed Oct 4, 2021
View reviewed changes
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.

This fix is good, but the one thing which is worry me is the bumping (up and down) size of the description - a perfect world select menu design is when the height of each select item is the same (in that case user can easily operate and estimate the actual set of the options). It is not critical, but there could be a couple of alternative solutions where we can keep the same height of the items: hover on the description with ... at the end to see the rest, etc. @mdefazio do you have any thought on this?

@mdefazio
Copy link
Contributor

mdefazio commented Oct 4, 2021

@YulNaumenko , I agree that ideally these would be consistent in height. However, with them being part of the content of the flyout as well as the overall length of the list, I think it's a good fix to simply show the description. It seems it's typically just 1-2 lines for each description. So the jumping of height should be minimal. However, if there are some that start having 4+ lines of description, then I think you're right that this raises a concern and likely needs a different solution.

YulNaumenko
YulNaumenko approved these changes Oct 4, 2021
View reviewed changes
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, since we will limit the description size.

chrisronline
chrisronline approved these changes Oct 6, 2021
View reviewed changes
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.

So many code changes! LGTM!

@ymao1
Copy link
Contributor Author

ymao1 commented Oct 6, 2021

@elasticmachine merge upstream

@ymao1 ymao1 added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 6, 2021
@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 6, 2021

💚 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
triggersActionsUi 758.7KB 758.7KB +12.0B

History

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

cc @ymao1

@ymao1 ymao1 merged commit e04de9b into elastic:master Oct 6, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 6, 2021
@ymao1 @kibanamachine
Wrapping text on create rule flyout (elastic#113305)
26eaf20 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Oct 6, 2021
Merged
@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 6, 2021

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

@ymao1 ymao1 deleted the alerting/long-descriptions-bug branch October 6, 2021 16:47
kibanamachine added a commit that referenced this pull request Oct 6, 2021
@kibanamachine @ymao1
Wrapping text on create rule flyout (#113305) (#114144)
31053f0 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: ymao1 <ying.mao@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@YulNaumenko YulNaumenko YulNaumenko approved these changes

+1 more reviewer

@chrisronline chrisronline chrisronline approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ymao1 ymao1

Labels

auto-backport Deprecated - use backport:version if exact versions are needed Feature:Alerting/RulesManagement Issues related to the Rules Management UX release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.16.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Alerting] Create rule flyout crops off longer rule descriptions

6 participants

@ymao1 @elasticmachine @mdefazio @kibanamachine @chrisronline @YulNaumenko