-
Notifications
You must be signed in to change notification settings - Fork 842
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
Convert cell styles to Emotion (row cells, header/footer cells, & cell checkboxes) #7631
Conversation
- I have no idea why it would apply to the content div instead of to the actual cell element :||| - !! this is a breaking DOM change and will need careful Kibana migration
+ use JS logic in `basic_table` to more accurately predict when custom actions are being rendered (vs trying to guess with CSS selectors)
- just inline it
+ move nested CSS from row styles to component + remove unnecessary font-weight/text-align CSS inherited from `@euiTableCell` mixin + remove `@euiTableCellCheckbox` mixin
- there's so much overridden and totally unnecessary overrides/CSS in here 😅 + remove now-unused `@euiTableCell` Sass mixin
- use BEM naming for more technical accuracy - seriously so much unnecessary CSS 🫠
b498498
to
2f86de7
Compare
2f86de7
to
8ffe0ba
Compare
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.
Mostly just questions and comments, but I think I did catch 1 problem.
margin-block-end: -${euiTheme.size.s}; | ||
|
||
/* Aligns contents of cells if header is empty */ | ||
.euiTableRowCell:not(:only-child) & { |
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.
I 👀 you refactoring two lines into 1.
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.
I'm going to approve to unblock you. I think I just had one suggestion that I'm pretty sure was an issue, but I'll trust you to fix that one. I'm good with these changes otherwise. Thank you for streamlining and cleaning up this CSS as well.
Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
ce803f7
to
413041a
Compare
Thanks a million for the review Jason, you rock! |
💚 Build Succeeded
History
cc @JasonStoltz |
Preview staging links for this PR:
|
Merging - remaining CI is just 1 flaky Cypress suite, rest passed |
…l checkboxes) (#7631) Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
…l checkboxes) (#7631) Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
`v93.6.0` ⏩ `v94.1.0` > [!important] > 👋 Hello everyone - this is a special release containing `EuiTable`'s conversion to Emotion, several long-overdue cleanups and breaking changes, and one or two fun new features. First, let's address the big questions: ### Q: I'm listed as a codeowner, how much should I manually QA/review? Answer: It depends on what exactly in your code changed, but _in general_ I would strongly suggest at least pulling this branch down and doing a quick visual smoke test of all tables (_note: **not** datagrids_) in your apps/plugins. You should not expect to see any major visual regressions. If your table contained any kind of custom styling or behavior (e.g. custom CSS, etc.) I **strongly** recommend taking more time to QA thoroughly to ensure your table still looks and behaves as expected. Teams with very manual or specific updates will be flagged below in comment threads. ### Q: When do I need to review by? This PR will be merged **after** 8.14FF. Because this upgrade touches so many files and teams, we're aiming for asking for an admin merge by EOD 4/18 regardless of full approval status. As always, you're welcome to ping us after merge if you find any issues later ([see our FAQ](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)), as you will have until 8.15FF to catch any bugs. ### Q: What breaking changes were made, and why? Here's a quick shortlist of all the changes made that affected the majority of the commits in this PR: #### <u>Removed several top-level table props</u> - The `responsive` prop has been removed in favor of the new `responsiveBreakpoint` prop (same `false` behavior as before) - The following props were removed from basic and in-memory tables: - `hasActions`, `isSelectable`, and `isExpandable` - These props were not used for functionality and were only used for styling tables in mobile/responsive views, which is not a best practice pattern we wanted for our APIs. Mobile tables are now styled correctly without needing consumers to pass these extra props. - `textOnly` - This prop was unused and had no meaningful impact on tables or table content. #### Removed hidden mobile vs. desktop DOM Previously, EUI would set classes that applied `display: none` CSS for content that was hidden for mobile vs. desktop. This is no longer the case, and content that only displays for mobile or only displays for desktop will no longer render to the DOM at all if the table is not in that responsive state. This is both more performant when rendering large quantities of cells/content, and simpler to write test assertions for when comparing what the user actually sees vs. what the DOM ‘sees’. (c3eeb08441e4b6efe6505e7cddaa87b540ddb259, 78cefcd954a7b46eaccd05e431b5e24dc86071a3) #### Removed direct usages of table `className`s EuiTable `classNames` no longer have any styles attached to them, so some instances where Kibana usages were applying the `className` for styles have been replaced with direct component usage or removed entirely (86ce80b). #### Custom table cell styles Any custom CSS for table cells was previously being applied to the inner `div.euiTableCellContent` wrapper. As of this latest release, the `className` and `css` props will now be applied directly to the outer `td.euiTableRowCell` element. If you were targeting custom styles table cells, please re-QA your styles to ensure everything still looks as expected. --- <details open><summary>Full changelog (click to collapse)</summary> ## [`v94.1.0-backport.0`](https://github.com/elastic/eui/releases/v94.1.0-backport.0) **This is a backport release only intended for use by Kibana.** **Bug fixes** - Fixed a visual text alignment regression in `EuiTableRowCell`s with the `row` header scope ([#7681](elastic/eui#7681)) **Accessibility** - Improved `EuiBasicTable` and `EuiInMemoryTable`'s selection checkboxes to have unique aria-labels per row ([#7672](elastic/eui#7672)) ## [`v94.1.0`](https://github.com/elastic/eui/releases/v94.1.0) - Updated `EuiTableHeaderCell` to show a subdued `sortable` icon for columns that are not currently sorted but can be ([#7656](elastic/eui#7656)) - Updated `EuiBasicTable` and `EuiInMemoryTable`'s `columns[].actions[]`'s to pass back click events to `onClick` callbacks as the second callback ([#7667](elastic/eui#7667)) ## [`v94.0.0`](https://github.com/elastic/eui/releases/v94.0.0) - Updated `EuiTable`, `EuiBasicTable`, and `EuiInMemoryTable` with a new `responsiveBreakpoint` prop, which allows customizing the point at which the table collapses into a mobile-friendly view with cards ([#7625](elastic/eui#7625)) - Updated `EuiProvider`'s `componentDefaults` prop to allow configuring `EuiTable.responsiveBreakpoint` ([#7625](elastic/eui#7625)) **Bug fixes** - `EuiBasicTable` & `EuiInMemoryTable` `isPrimary` actions are now correctly shown on mobile views ([#7640](elastic/eui#7640)) - Table `mobileOptions`: ([#7642](elastic/eui#7642)) - `mobileOptions.align` is now respected instead of all cells being forced to left alignment - `textTruncate` and `textOnly` are now respected even if a `render` function is not passed **Breaking changes** - Removed unused `EuiTableHeaderButton` component ([#7621](elastic/eui#7621)) - Removed the `responsive` prop from `EuiTable`, `EuiBasicTable`, and `EuiInMemoryTable`. Use the new `responsiveBreakpoint` prop instead ([#7625](elastic/eui#7625)) - The following props are no longer needed by `EuiBasicTable` or `EuiInMemoryTable` for responsive table behavior to work correctly, and can be removed: ([#7632](elastic/eui#7632)) - `isSelectable` - `isExpandable` - `hasActions` - Removed the `showOnHover` prop from `EuiTableRowCell` / `EuiBasicTable`/`EuiInMemoryTable`'s `columns` API. Use the new actions `columns[].actions[].showOnHover` API instead. ([#7640](elastic/eui#7640)) - Removed top-level `textOnly` prop from `EuiBasicTable` and `EuiInMemoryTable`. Use `columns[].textOnly` instead. ([#7642](elastic/eui#7642)) **DOM changes** - `EuiTable` mobile headers no longer render in the DOM when not visible (previously rendered with `display: none`). This may affect DOM testing assertions. ([#7625](elastic/eui#7625)) - `EuiTableRowCell` now applies passed `className`s to the parent `<td>` element, instead of to the inner cell content `<div>`. ([#7631](elastic/eui#7631)) - `EuiTableRow`s rendered by basic and memory tables now only render a `.euiTableRow-isSelectable` className if the selection checkbox is not disabled ([#7632](elastic/eui#7632)) - `EuiTableRowCell`s with `textOnly` set to `false` will no longer attempt to apply the `.euiTableCellContent__text` className to child elements. ([#7641](elastic/eui#7641)) - `EuiTableRowCell` no longer renders mobile headers to the DOM unless the current table is displaying its responsive view. ([#7642](elastic/eui#7642)) - `EuiTableHeaderCell` and `EuiTableRowCell` will no longer render in the DOM at all on mobile if their columns' `mobileOptions.show` is set to `false`. ([#7642](elastic/eui#7642)) - `EuiTableHeaderCell` and `EuiTableRowCell` will no longer render in the DOM at all on desktop if their columns' `mobileOptions.only` is set to `true`. ([#7642](elastic/eui#7642)) **CSS-in-JS conversions** - Converted `EuiTable`, `EuiTableRow`, `EuiTableRowCell`, and all other table subcomponents to Emotion ([#7654](elastic/eui#7654)) - Removed the following `EuiTable` Sass variables: ([#7654](elastic/eui#7654)) - `$euiTableCellContentPadding` - `$euiTableCellContentPaddingCompressed` - `$euiTableCellCheckboxWidth` - `$euiTableHoverColor` - `$euiTableSelectedColor` - `$euiTableHoverSelectedColor` - `$euiTableActionsBorderColor` - `$euiTableHoverClickableColor` - `$euiTableFocusClickableColor` - Removed the following `EuiTable` Sass mixins: ([#7654](elastic/eui#7654)) - `euiTableActionsBackgroundMobile` - `euiTableCellCheckbox` - `euiTableCell` </details>
Summary
.euiTableCellContent
to Emotion because that is going to be its own large PR - I plan on very judiciously refactoring the heck out of that 😅As always, this is a lot, and there was a lot of CSS cleanups that happened, so I recommend following along by commit!
euiTableCell
andeuiTableCellCheckbox
(no usages)className
changes (1065b0b): (4 usages to manually QA during the upgrade):QA
General checklist
- [ ] Checked for accessibility including keyboard-only and screenreader modes