-
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
[Discover] Add wrapping to field list on sidebar #71312
Conversation
49f627d
to
6ef78cb
Compare
Pinging @elastic/kibana-app (Team:KibanaApp) |
@elasticmachine merge upstream |
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.
Code LGTM, this will be a big improvement for users with long fieldnames. My question here is, if it needs more adaption. Compared to Lens there's less available minimum width available, maybe this should be increased? CC: @AlonaNadler
I feel like a lot of work is going into duplicating the Lens display of fields when, at this point, we should probably just make a re-usable component. They can still have a few differentiating characteristics like the "Add" button but display-wise it is taking just as long to replicate the same styles. I'd say, that this PR is a quick win to get wrapping field names, but the next PR should be to create a component for use in both applications. |
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.
Works as described. Quick win for non-truncated field names. Just a few comments on the SASS
src/plugins/discover/public/application/components/sidebar/discover_sidebar.scss
Outdated
Show resolved
Hide resolved
text-align: left; | ||
} | ||
|
||
.dscSidebarField__name { | ||
margin-left: $euiSizeS; | ||
flex-grow: 1; | ||
word-break: break-word; | ||
padding-right: $euiSizeXS * 0.25; |
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.
1px padding?
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.
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.
👍 Gotcha. Since the math just ends up being 1px
your fine to just say 1px
it's anything above that that requires math on our sizing variables
src/plugins/discover/public/application/components/sidebar/discover_sidebar.scss
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!
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
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.
Code LGTM, hurray for the quick win 🎉 , tested locally in Chrome, Firefox, Safari, MacOs 10.14.6, works as expected
Discussed with Matthias that we'll take care of the sidebar width in another PR. |
Summary
This PR removes truncation from fields and makes them wrap instead. Following what's being done on Lens, wrapping happens on the
.
whenever possible.Also added a rounded border to each item and adjusted spacing.
Fixes #69814
Checklist
Delete any items that are not applicable to this PR.
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers