Final tables PR: cleanup, tests, stories #7642
Merged
Conversation
This file contains 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
cee-chen
force-pushed
the
emotion/table-9
branch
from
bea3478 to
bc8ebbd
Compare
…one` CSS - better performance at scale, and also generally cleaner code
- previous PR feedback - make it clearer how the example is working - move the giant code block to the `snippet` tab instead of making users scroll past it - the `truncateText`/`textOnly` limitation no longer applies since the last commit refactor + default row/card count to a smaller initial size for easier viewing
+ fix header/row cell tests not being in a component `describe` block
- and just use conditional chain operator instead & explicit falsey checks
cee-chen
force-pushed
the
emotion/table-9
branch
from
bc8ebbd to
ffe02fd
Compare
- add more actual prop documentation & missing `@default`s & re-order some props (e.g. `sortable` and `readOnly` go together) - DRY out very repetitive props shared between column types via `Pick<>` NOTE: This requires changing from an interface to a type and creating a prop 'component' in order for our props tables to correctly render the picked type. IMO this is worth the extra hassle since 1. we've already had prop docs fall out of date because 1. type was changed and the others were forgotten, and 2. we're moving our docs soon in any case
…ting - still not totally sure why this prop isn't documented tbh
cee-chen
force-pushed
the
emotion/table-9
branch
from
ffe02fd to
634ea5c
Compare
- fix missing initial boolean toggle on new `hasSelection` prop - update `hasActions` to a selection dropdown to account for new `custom` option, and add a custom action for testing
+ fix misc missing spaces in memory table docs
- appeared to be completely unused, noticed/QA'd via Storybook
- yay for storybooks helping us find bugs
cee-chen
force-pushed
the
emotion/table-9
branch
from
634ea5c to
95aa6f0
Compare
|
This PR contains breaking changes. The opener of this pull request is asked to perform the following due diligence steps below, to assist EUI in our next Kibana upgrade:
|
mgadewoll
reviewed
Apr 4, 2024
src-docs/src/views/tables/in_memory/in_memory_controlled_pagination_section.js
Show resolved
Hide resolved
1Copenut
reviewed
Apr 4, 2024
There was a problem hiding this comment.
I tested the PR against production in desktop and mobile widths using four pairings:
- MacOS Safari + VoiceOver
- Win10 + Firefox + NVDA
- Win10 + Chrome + NVDA
- Win10 + Chrome + JAWS
All pairings retained the previous table-centric behavior and announcements (no regressions). I was able to traverse through the content and heard the rows announced because of the th[scope="row"] markup on the first columns.
Refactoring to ignore renders vs. hiding with display: none; feels transparent for screen readers. CSS display: none removes nodes from the accessibility tree, effectively having the same effect as not rendering. This is a win all the way around.
Thanks for the heads up to review!
💚 Build Succeeded
History
|
|
Preview staging links for this PR:
|
cee-chen
added a commit
that referenced
this pull request
Apr 5, 2024
This was referenced Apr 5, 2024
mistic
pushed a commit
to elastic/kibana
that referenced
this pull request
Apr 18, 2024
`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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 is primarily a polish pass, containing a final set of (opinionated/slightly spicy) responsive cleanups (5634d4c), a final Storybook/tests/docs pass, and some last-minute bugfixes.
A lot of the line diffs are caused by new stories/tests/docs and not changes to source code. As always, I strongly recommend reviewing by commit and turning off whitespace changes, especially for the commits that add a
describe()block (i.e. lots of indentation changes) to tests.textOnlyprop removal (Typescript should catch most of these in the upgrade):EuiTableRowCelldirectly, which is still valid usagemobileOnly.show/onlyis a potentially spicy one that may affect test assertions that rely on the DOM being there (but hidden viadisplay: none). We'll have to see in the upgrade how that goes, but I lean toward it being right call to make because it's significantly cleaner and more performant at scale. Would be interested in hearing other folks' thoughts!QA
Storybooks QA: confirm the below table stories have no noticeable UI/UX bugs and prop controls work as expected
Docs QA:
General checklist
- [ ] Checked for accessibility including keyboard-only and screenreader modes- Will have Trevor do a final screen reader pass either when he gets back or on the feature branch PR into main@defaultif default values are missing) and playground toggles- [ ] Checked Code Sandbox works for any docs examplesand cypress tests