[EuiDataGrid] Update to use EuiTextBreakTruncate + cell DOM & CSS cleanup#7255
Merged
cee-chen merged 16 commits intoelastic:mainfrom Oct 9, 2023
Merged
[EuiDataGrid] Update to use EuiTextBreakTruncate + cell DOM & CSS cleanup#7255cee-chen merged 16 commits intoelastic:mainfrom
EuiTextBreakTruncate + cell DOM & CSS cleanup#7255cee-chen merged 16 commits intoelastic:mainfrom
Conversation
EuiTextBreakTruncate componentEuiTextBreakTruncate component
- in favor of a more agnostic height-type util - We'll be replacing the line count styles with the new `EuiTextBlockTruncate` component - NOTE: the `height: 100%` style needs to be retained for `lineCount` and `numerical` height types, hence the new util that will be passed to a CSS class
- Remove unnecessary `flex-grow: 1` - isn't doing anything + start the process of moving out the text truncation/breaking CSS to their className utils
- allows us to remove torturous selector and repeat CSS in favor of using explicit className utils - `isDefinedHeight` is no longer used, remove it
EuiTextBreakTruncate componentEuiTextBreakTruncate component + cell DOM & CSS cleanup
1e300f5 to
4c4dc57
Compare
EuiTextBreakTruncate component + cell DOM & CSS cleanupEuiTextBreakTruncate + cell DOM & CSS cleanup
- `overflow` is already set by text truncate component - `height: 100%` doesn't appear to do anything or be useful. if for some reason it is needed in the future, we can always re-add it
- in favor of passing condition/setting conditional styling logic directly in the cell actions component
- pass var(s) instead of re-getting it twice
- move all logic related to content to the `EuiDataGridCellContent` component, including the wrapper let the parent cell pass down any needed refs + actions
- remove extremely confusing `__expand` class names in favor of using the height type (that determines the kind of behavior the content has, e.g. flex vs absolutely positioned, etc) - Make sure only the heights that need certain CSS have that CSS (the non-default-height cells don't need display flex)
- move flex CSS specific to default height cells to its selector, vs. the baseline actions component
…ts that have overflowing content + Add Cypress regression tests to ensure popovers are rendering from the correct anchor/position
4c4dc57 to
c5343af
Compare
Contributor
Author
|
@1Copenut I'd recommend focusing on the QA for this PR and only lightly skimming or skipping the code review 😅 Primarily what I want to be sure of is that cell display (when toggling to different height options) is the same as production, and that cell popover anchoring/positioning is also the same as production without regressions. EDIT: Also no rush on reviewing this, I'm shooting to merge it in after next week's release to reduce churn (since we already have a few spicier changes going in this week, I want to spread this one out) |
|
Preview staging links for this PR:
|
Collaborator
💚 Build Succeeded
History
|
1Copenut
approved these changes
Oct 6, 2023
Contributor
1Copenut
left a comment
There was a problem hiding this comment.
👍 I reviewed code briefly as you suggested @cee-chen and focused most of my efforts on manual QA. I ran through each of the suggested steps in the four evergreen browsers and confirmed the correct readout behavior in VoiceOver, NVDA, and JAWS with preferred browser pairings. Solid PR!
cee-chen
added a commit
to cee-chen/kibana
that referenced
this pull request
Oct 17, 2023
- to account for cell DOM refactored in elastic/eui#7255
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
Oct 19, 2023
`v89.0.0`⏩`v89.1.0` This upgrade also contains an EuiDataGrid refactor (elastic/eui#7255) not listed in the changelog (as no end-user functionality or props changed as a result of the refactor). The unlisted changes should only affect DOM and `className` usages in Kibana (primarily CSS overrides and test selectors). --- ## [`89.1.0`](https://github.com/elastic/eui/tree/v89.1.0) - Added `tokenVectorSparse` token and updated `tokenDenseVector` token (now named `tokenVectorDense`). ([#7282](elastic/eui#7282)) **CSS-in-JS conversions** - Reduced default CSS prefixes generated by Emotion to only browsers supported by EUI (latest evergreen browsers). This can be customized by passing your own Emotion cache to `EuiProvider`. ([#7272](elastic/eui#7272))
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR started as just updating EuiDataGrid to use the new
EuiTextBreakTruncatecomponent... and then I kept staring at the code going "ugh this is so hard to read/understand" and then several hours later I'd done my favorite thing, which is cleaning up code (and discovering a bug or two along the way) 🤣I split up the commits by emoji to signify which items were refactors (🔥). I also recommend reading the commit messages as I tried to explain my reasoning for certain changes as I went along.
height: 40cells that have overflowing content - the popover is positioning off the overflowed content and not the cell itself)This is a refactor PR and no functionality or end-user experience should have degraded as a result of this PR. If that is not the case / if you find a regression, please let me know!
QA
Regression testing:
Display optionsbutton icon in the top right hand corner, and set theRow heightcontrol toAuto fitCustom(should default to 2 lines per row)euiTextBreakTruncateclassName is presentGeneral checklist
- [ ] Checked in both light and dark modes- [ ] Checked in mobileand cypress tests