Skip to content
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

Upgrade EUI to v94.1.0 (major EuiTable refactors) #180514

Merged
merged 37 commits into from
Apr 18, 2024

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Apr 10, 2024

v93.6.0v94.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), 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:

Removed several top-level table props

  • 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 classNames

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.


Full changelog (click to collapse)

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 EuiTableRowCells with the row header scope (#7681)

Accessibility

  • Improved EuiBasicTable and EuiInMemoryTable's selection checkboxes to have unique aria-labels per row (#7672)

v94.1.0

  • Updated EuiTableHeaderCell to show a subdued sortable icon for columns that are not currently sorted but can be (#7656)
  • Updated EuiBasicTable and EuiInMemoryTable's columns[].actions[]'s to pass back click events to onClick callbacks as the second callback (#7667)

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)
  • Updated EuiProvider's componentDefaults prop to allow configuring EuiTable.responsiveBreakpoint (#7625)

Bug fixes

  • EuiBasicTable & EuiInMemoryTable isPrimary actions are now correctly shown on mobile views (#7640)
  • Table mobileOptions: (#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)
  • Removed the responsive prop from EuiTable, EuiBasicTable, and EuiInMemoryTable. Use the new responsiveBreakpoint prop instead (#7625)
  • The following props are no longer needed by EuiBasicTable or EuiInMemoryTable for responsive table behavior to work correctly, and can be removed: (#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)
  • Removed top-level textOnly prop from EuiBasicTable and EuiInMemoryTable. Use columns[].textOnly instead. (#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)
  • EuiTableRowCell now applies passed classNames to the parent <td> element, instead of to the inner cell content <div>. (#7631)
  • EuiTableRows rendered by basic and memory tables now only render a .euiTableRow-isSelectable className if the selection checkbox is not disabled (#7632)
  • EuiTableRowCells with textOnly set to false will no longer attempt to apply the .euiTableCellContent__text className to child elements. (#7641)
  • EuiTableRowCell no longer renders mobile headers to the DOM unless the current table is displaying its responsive view. (#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)
  • EuiTableHeaderCell and EuiTableRowCell will no longer render in the DOM at all on desktop if their columns' mobileOptions.only is set to true. (#7642)

CSS-in-JS conversions

  • Converted EuiTable, EuiTableRow, EuiTableRowCell, and all other table subcomponents to Emotion (#7654)
  • Removed the following EuiTable Sass variables: (#7654)
    • $euiTableCellContentPadding
    • $euiTableCellContentPaddingCompressed
    • $euiTableCellCheckboxWidth
    • $euiTableHoverColor
    • $euiTableSelectedColor
    • $euiTableHoverSelectedColor
    • $euiTableActionsBorderColor
    • $euiTableHoverClickableColor
    • $euiTableFocusClickableColor
  • Removed the following EuiTable Sass mixins: (#7654)
    • euiTableActionsBackgroundMobile
    • euiTableCellCheckbox
    • euiTableCell

@cee-chen cee-chen changed the title Eui/v94.1.0 Upgrade EUI to v94.1.0 (major EuiTable refactors) Apr 10, 2024
@cee-chen cee-chen force-pushed the eui/v94.1.0 branch 3 times, most recently from 37746b5 to 7ac2663 Compare April 10, 2024 18:36
@elastic elastic deleted a comment from kibana-ci Apr 10, 2024
@cee-chen cee-chen added v8.15.0 release_note:skip Skip the PR/issue when compiling release notes EUI labels Apr 10, 2024
@cee-chen cee-chen force-pushed the eui/v94.1.0 branch 5 times, most recently from 35f8a43 to 32da71e Compare April 11, 2024 20:44
cee-chen added 13 commits April 11, 2024 13:46
- same `false` behavior, but more extensible if needed
- now detected automatically in `columns`, there's no need to pass this prop manually
- as far as I can tell this util isn't used anywhere else, so I'm deleting it entirely
- this is now automatically detected by the presence of the `itemIdToExpandedRowMap` prop and no longer needs to be passed manually
- was confused about this as the condition for `itemIdToExpandedRowMap` wasn't the same as `isExpandable`, so I updated the row map to support both
- this is now automatically detected by the presence of the `selectable` prop and no longer needs to be passed manually
- basic validation against `selection` conditionals, if they existed

- either way, this only affects responsive table views

+ remove unit test that no longer applies
- `isSelectable` now indicates whether the checkbox is disabled or enabled; `hasSelection` (new prop) determines visual checkbox affordance
- remove unnecessary props that already default to false

- various syntax nits

- small CSS fixes
- they no longer have styles attached to them and thus no longer do anything

- replace with direct component usage instead (or in some cases remove if they're not doing anything)
- replace with custom CSS instead for now, with a comment
- now that EuiBasicTable's default actions supports passing the item to `href` as well as the click event to `onClick`
Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data Discovery changes LGTM 👍

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @cee-chen! Looks good from my perspective. Approving.

@elastic/kibana-management: In testing, I noticed that the "Type" column is now truncated by default on the saved object page in stack management. This is due to the new addition of the sortable icon that appends each sortable column header. Ya'll may want to increase the width of this column slightly so that the text isn't always truncated.

CleanShot 2024-04-17 at 10 32 46

- `:first-child` CSS no longer works well since `EuiBasicTable` no longer adds a `<div>` wrapper around the `<table>`

+ additionally fix Errors table to align the pagination bar to the bottom, like the Transactions and Dependencies tables
…new sortable icon

+ use first custom `responsiveBreakpoint` to handle table with lots of columns!! 🎉
@cee-chen
Copy link
Contributor Author

cee-chen commented Apr 18, 2024

@MichaelMarcialis I went ahead and fixed that in this PR - let me know if this looks good to you, and also if you find any other similar issues!

@jennypavlova Thank you so much for the help in getting Obs data into my local, I should have fixed all the issues in the screenshots in #180514 (review). I additionally fixed a small visual bug already in prod where the Errors table pagination wasn't aligned the same way that the Transactions and Dependencies tables were.

Also the Hosts overview table is now the first table to use the new responsiveBreakpoint feature - it has a lot of columns and text was cutting off very fast even on prod, so I changed it to collapse down to cards much sooner. Let me know what you think! 🎉

@cee-chen
Copy link
Contributor Author

👋

@elastic/security-entity-analytics
@elastic/security-defend-workflows
@elastic/security-threat-hunting-explore
@elastic/security-generative-ai
@elastic/ml-ui
@elastic/obs-ux-logs-team
@elastic/appex-sharedux
@elastic/kibana-esql
@elastic/enterprise-search-frontend
@elastic/obs-ai-assistant
@elastic/logstash
@elastic/kibana-visualizations

One last ping for QA and review requests by EOD Thursday - we'll be asking for an admin merge if not. As always, even if you catch any visual bugs post-merge, the EUI team is more than happy to help quickly fix any issues after the fact.

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested usage of EuiBasicTable for Lens. LGTM 👌🏼

Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀 @cee-chen Thank you very much for all the fixes! I tested it and all the tables look good ✨

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Threat Hunting Explore - Relevant usages of EuiTable tested, all good.
LGTM

@semd
Copy link
Contributor

semd commented Apr 18, 2024

I went a bit too fast with the approval, I just realized we have an issue very similar to this one in our Data Quality Dashboard page.

results_column_issue

We only need to increase the width here:

To 70px:

result_column_fixed

@cee-chen can you apply the change in this PR?

@cee-chen
Copy link
Contributor Author

cee-chen commented Apr 18, 2024

@semd Updated in ac4aa7d! Thank you for catching that!

Copy link
Member

@sphilipse sphilipse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smoke-tested Search changes, LGTM!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
security 519 518 -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 412.6KB 412.6KB -36.0B
alerting 91.6KB 91.5KB -14.0B
apm 3.2MB 3.2MB +136.0B
canvas 1016.4KB 1016.4KB -13.0B
cases 475.8KB 475.7KB -30.0B
cloudSecurityPosture 451.3KB 451.2KB -29.0B
crossClusterReplication 148.0KB 147.9KB -32.0B
dashboard 395.5KB 395.5KB -16.0B
datasetQuality 156.2KB 156.2KB -16.0B
dataViewEditor 48.6KB 48.6KB +10.0B
dataViewManagement 139.2KB 139.1KB -32.0B
dataVisualizer 656.7KB 656.6KB -37.0B
discover 638.0KB 638.0KB -4.0B
enterpriseSearch 2.7MB 2.7MB -140.0B
eventAnnotationListing 198.6KB 198.5KB -16.0B
filesManagement 90.5KB 90.5KB -16.0B
fleet 1.3MB 1.3MB -160.0B
graph 387.7KB 387.7KB -16.0B
indexManagement 620.0KB 619.7KB -330.0B
infra 1.5MB 1.5MB +172.0B
ingestPipelines 367.3KB 367.3KB -16.0B
inspector 28.1KB 28.1KB -22.0B
lens 1.4MB 1.4MB +20.0B
logstash 32.5KB 32.5KB -15.0B
maps 2.9MB 2.9MB -16.0B
ml 4.1MB 4.1MB -536.0B
osquery 1.0MB 1.0MB -51.0B
remoteClusters 78.1KB 78.1KB -16.0B
reporting 71.2KB 71.1KB -16.0B
searchPlayground 151.9KB 151.9KB -30.0B
security 584.5KB 583.3KB -1.2KB
securitySolution 14.6MB 14.6MB -826.0B
serverlessSearch 455.3KB 455.3KB -14.0B
slo 723.7KB 723.8KB +10.0B
snapshotRestore 261.3KB 261.2KB -64.0B
spaces 175.1KB 175.0KB -14.0B
synthetics 1.0MB 1.0MB -223.0B
textBasedLanguages 164.7KB 164.7KB +10.0B
transform 393.0KB 392.9KB -46.0B
triggersActionsUi 1.6MB 1.6MB -142.0B
uiActionsEnhanced 135.8KB 135.7KB -41.0B
unifiedDocViewer 60.6KB 60.8KB +132.0B
uptime 462.7KB 462.6KB -70.0B
ux 169.8KB 169.8KB +10.0B
visualizations 274.3KB 274.3KB -16.0B
watcher 165.1KB 165.1KB -16.0B
total -3.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 406.3KB 406.3KB +17.0B
kbnUiSharedDeps-css 273.7KB 254.2KB -19.6KB
kbnUiSharedDeps-npmDll 6.3MB 6.3MB +11.6KB
observabilityAIAssistant 47.0KB 46.7KB -406.0B
total -8.3KB

History

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

@mistic mistic merged commit cbc68cd into elastic:main Apr 18, 2024
48 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 18, 2024
@cee-chen cee-chen deleted the eui/v94.1.0 branch April 26, 2024 17:39
jgowdyelastic added a commit that referenced this pull request Apr 29, 2024
cee-chen added a commit that referenced this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:skip This commit does not require backporting EUI release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Obs AI Assistant Observability AI Assistant Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.