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 v91.3.1 #173569

Merged
merged 33 commits into from
Jan 5, 2024
Merged

Upgrade EUI to v91.3.1 #173569

merged 33 commits into from
Jan 5, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Dec 18, 2023

v91.0.0-backport.0v91.3.1

⚠️ The largest set of changes in this PR that touch source code (as opposed to test code) are related to several EuiDataGrid redesigns, particularly around the toolbar, column cell headers, and cell actions. We strongly recommend QAing your EuiDataGrid usages, especially if you have custom CSS styling on data grid cells.

Changes Screencap
Cell actions and popover
Column headers
Toolbar

v91.3.1

Bug fixes

  • Moved EuiDataGrid's header cells' dataGridHeaderCellActionButton test subject attribute from to the clickable button, for easier E2E testing (#7427)
  • Fixed EuiBasicTable/EuiInMemoryTable actions to correctly show as disabled when rows are being selected (#7428)

v91.3.0

  • Added esqlVis, pipeBreaks, and pipeNoBreaks icon glyphs. (#7399)
  • Updated EuiDataGridSchemaDetector's comparator arguments to include entry indexes (#7406)

v91.2.0

  • Added endpoint glyph to EuiIcon (#7383)

Bug fixes

  • Fixed a bug with EuiSelectables with custom truncationProps, where scrollbar widths were not being accounted for (#7392)

v91.1.0

  • Updated EuiDataGrid cell actions to display above cells instead of within them, to avoid content clipping issues (#7343)
  • Updated EuiDataGrid cell expansion popovers to sit on top of cells instead of below/next to them (#7343)
  • Updated EuiListGroupItem to render an external icon and screen reader affordance for links with target set to to _blank (#7352)
  • Updated EuiListGroupItem with a new external prop, which allows enabling or disabling the new external link icon (#7352)
  • Updated EuiText to no longer set any opinionated styles on child <img> tags - use EuiImage for image display within text instead (#7360)
  • Improved EuiBasicTable/EuiInMemoryTables mobile UI for custom actions (#7361)
  • Added a new EuiDataGridToolbarControl subcomponent, which is useful for rendering your own custom EuiDataGrid toolbar buttons while matching the look of the default controls (#7369)
  • Updated EuiDataGrid's toolbar controls to show active/current counts in badges, and updated the Columns button icon (#7369)
  • Updated EuiButtonEmpty to allow passing false to textProps, which allows rendering custom button content without an extra text wrapper (#7369)
  • Updated EuiDataGrid column header cells to show the sort arrow after the heading text, instead of before (#7371)
  • Updated EuiDataGrid's column header actions icon from a chevron to boxesVertical (#7371)
  • Updated the actions column in EuiBasicTable and EuiInMemoryTables. Alongside name, the description, href, and data-test-subj properties now also accept an optional callback that the current item will be passed to (#7373)
  • Updated EuiContextMenuItem with a new toolTipProps prop (#7373)
  • EuiSelectable now allows configurable text truncation via listProps.truncationProps (#7388)
  • EuiTextTruncate now supports a new calculationDelayMs prop for working around font loading or layout shifting scenarios (#7388)

Bug fixes

  • Fixed incorrect EuiPopover positioning calculations when hasArrow was set to false (#7343)
  • Fixed EuiSuperSelect to render options with falsy values (false, 0, and ''), but not nullish values (undefined or null) (#7362)
  • Fixed EuiSuperSelect's typing to allow non-string values (e.g., booleans or numbers) (#7362)
  • Fixed EuiDataGrid's numeric and currency column heading cells to be correctly right-aligned (#7371)
  • Fixed EuiBasicTable and EuiInMemoryTable actions not showing tooltip descriptions when rendered in the all actions popover menu (#7373)
  • Fixed missing underlines on EuiContextMenu link hover (#7373)
  • Fixed visual text truncation of EuiBreadcrumbs with popoverContent (#7375)
  • Fixed EuiFormRows with hasEmptyLabelSpace being very slightly off in vertical alignment (#7380)

Deprecations

  • Deprecated EuiContextMenuItem's toolTipTitle prop. Use toolTipProps.title instead (#7373)
  • Deprecated EuiContextMenuItem's toolTipPosition prop. Use toolTipProps.position instead (#7373)

Accessibility

  • Fixed custom EuiBasicTable/EuiInMemoryTable rendering nested interactive custom actions (#7361)
  • Fixed EuiBasicTable and EuiInMemoryTable actions not correctly reading out action descriptions to screen readers (#7373)
  • Fixed EuiBasicTable and EuiInMemoryTable primary actions not visibly appearing on keyboard focus (#7373)

- `textProps` can now be false, so we need the specific falsy check instead of using optional chaining
- the component now allows setting non-nullish falsy values (e.g. false, 0, etc) as option values, so specifying `string` is needed here
- hash changed due to removed child `img` styles
- now that we're no longer opinionatedly adding margins to `<img>` tags within EuiText

- see elastic/eui#7356 (comment)
- icon is still there, but the text is not if `target="_blank"` is not set
- EuiBasicTable no longer disables the `...` icon if all actions are disabled, only if the row is selected
- this is effectively the same UX as the individual actions in the popover are still disabled, but users now know specifically what actions they don't have access to
… is selected

- a previous test that was selecting rows was polluting subsequent tests

- to be clear, the previous UX was already that the actions would be disabled on selection - the EUI fix was to remove the disabled actions from the DOM, which the test was asserting on
… for new redesign

+ replace theme token usage with new preferred Emotion-inferred theme, which involves adding an `emotion.d.ts` file (https://emotion.sh/docs/typescript#define-a-theme)
- not totally sure why this changed slightly, but my best guess is the header redesign
- should no longer be necessary with new design
@cee-chen cee-chen marked this pull request as ready for review December 21, 2023 02:47
@cee-chen cee-chen requested review from a team as code owners December 21, 2023 02:47
@MichaelMarcialis
Copy link
Contributor

@PhilippeOberti Good catch! @MichaelMarcialis, any design thoughts/suggestions here? My gut reaction would be to strongly suggest moving the tooltip position for your content to use position="bottom" & align the tooltip below the cell, instead of above/top.

Thanks, @cee-chen. As discussed in today's EUI weekly meeting, I think the best path forward is to:

  1. Utilize a tooltip position other than top for any tooltips within a data grid cell (as you also mentioned above).
  2. Change the elements' z-index so that the tooltips will always appear behind the data grid cell actions. This is for the case that the tooltip is rendered in close proximity to the edge of the user's viewport, and forcing the tooltip to position itself to the top again. This will prevent the cell actions from being partially obscured by the tooltip. I don't believe the overlapping of the actions above the tooltip will cause any usability issues, as the tooltip's padding should prevent any covering of the text within.

Copy link
Contributor

@gergoabraham gergoabraham left a comment

Choose a reason for hiding this comment

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

Defend Workflow related changes look good! 🚢

a big thank you for the cypress test updates 🙇

@cee-chen cee-chen requested a review from a team as a code owner January 4, 2024 17:59
@cee-chen
Copy link
Member Author

cee-chen commented Jan 4, 2024

@PhilippeOberti - so unfortunately the z-index approach we wanted to take doesn't work due to tooltips being portalled to the document body 😞 I settled for changing the position to bottom for all tooltips, and bumping up the offset of top-aligned tooltips (that don't have enough room at the bottom) so that actions are at least more visible:

datagridtooltip

(ac3bef9) That's the best I can do in this PR for now - if your team still wants a better longer-term solution, please let us/@MichaelMarcialis know so we can brainstorm something!

cee-chen and others added 2 commits January 4, 2024 10:55
- restore left alignment of header cells

- fix vertical center alignment of cell contents

- add more left spacing to custom toolbar data

- give timestamp tooltips the same workaround for new cell actions design that Security timeline received
Comment on lines +55 to 58
/* EUI QUESTION: Why is this being done via CSS instead of setting isExpandable: false in the columns API? */
& .euiDataGridRowCell__actions > .euiDataGridRowCell__expandCell {
display: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing this out! I actually wasn't aware about this option, I'll update it to use isExpanded in a follow up PR

Copy link
Member Author

@cee-chen cee-chen Jan 4, 2024

Choose a reason for hiding this comment

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

Thanks!! 🙇 I was able to take a peek at this when I was fixing the UI locally. display: none isn't visually breaking in this case, but it could be a very minor accessibility issue for keyboards users with no screen readers (since keyboard users can't access cell actions except via the expansion popover).

If it's not a blocker, we'd love for this CSS to simply be removed, but we'll leave it up to you all/another PR!

Copy link
Contributor

@opauloh opauloh left a comment

Choose a reason for hiding this comment

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

cloud security changes LGTM 🚀🚀🚀

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
aiops 387.2KB 387.2KB +8.0B
apm 3.1MB 3.1MB +1.0B
cloudSecurityPosture 455.2KB 455.4KB +145.0B
data 46.9KB 46.9KB +16.0B
discover 594.6KB 594.4KB -172.0B
exploratoryView 203.3KB 203.3KB +1.0B
fileUpload 956.6KB 956.6KB +1.0B
infra 1.3MB 1.3MB +9.0B
lens 1.4MB 1.4MB -12.0B
ml 3.6MB 3.6MB +417.0B
monitoring 462.2KB 462.2KB +1.0B
securitySolution 11.3MB 11.4MB +586.0B
serverlessSearch 424.3KB 424.2KB -48.0B
synthetics 843.9KB 843.9KB +1.0B
transform 402.5KB 402.9KB +415.0B
uptime 458.4KB 458.4KB +1.0B
total +1.3KB

Page load bundle

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

id before after diff
core 378.0KB 377.8KB -200.0B
inspector 22.0KB 22.0KB +8.0B
kbnUiSharedDeps-css 255.8KB 258.0KB +2.2KB
kbnUiSharedDeps-npmDll 6.2MB 6.2MB +1.1KB
logsShared 219.1KB 219.1KB +1.0B
observability 100.7KB 100.8KB +1.0B
total +3.1KB
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/ml-data-grid 3 4 +1
@kbn/unified-data-table 5 4 -1
total -0

Total ESLint disabled count

id before after diff
@kbn/ml-data-grid 3 4 +1
@kbn/unified-data-table 5 4 -1
total -0

History

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

Copy link
Contributor

@PhilippeOberti PhilippeOberti 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 for fixing the weird tooltip issue. The solution you have now seems actually pretty good!

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Response Ops changes LGTM

@cee-chen cee-chen merged commit 7357af5 into elastic:main Jan 5, 2024
39 checks passed
@cee-chen cee-chen deleted the eui-v91.3.0 branch January 5, 2024 16:22
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 5, 2024
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
`v91.0.0-backport.0`⏩`v91.3.1`

⚠️ The largest set of changes in this PR that touch source code (as
opposed to test code) are related to several **EuiDataGrid** redesigns,
particularly around the toolbar, column cell headers, and cell actions.
We **strongly** recommend QAing your EuiDataGrid usages, **especially**
if you have custom CSS styling on data grid cells.

| Changes | Screencap |
|--------|--------|
| Cell actions and popover | <img
src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0"
width="240" alt=""> |
| Column headers | <img
src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2"
alt="" width="360"> |
| Toolbar | <img
src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3"
alt="" width="240"> |

---

## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1)

**Bug fixes**

- Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton`
test subject attribute from to the clickable button, for easier E2E
testing ([elastic#7427](elastic/eui#7427))
- Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as
disabled when rows are being selected
([elastic#7428](elastic/eui#7428))

## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0)

- Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs.
([elastic#7399](elastic/eui#7399))
- Updated `EuiDataGridSchemaDetector`'s comparator arguments to include
entry indexes ([elastic#7406](elastic/eui#7406))

## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0)

- Added `endpoint` glyph to `EuiIcon`
([elastic#7383](elastic/eui#7383))

**Bug fixes**

- Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where
scrollbar widths were not being accounted for
([elastic#7392](elastic/eui#7392))

## [`v91.1.0`](https://github.com/elastic/eui/releases/tag/v91.1.0)

- Updated `EuiDataGrid` cell actions to display above cells instead of
within them, to avoid content clipping issues
([elastic#7343](elastic/eui#7343))
- Updated `EuiDataGrid` cell expansion popovers to sit on top of cells
instead of below/next to them
([elastic#7343](elastic/eui#7343))
- Updated `EuiListGroupItem` to render an external icon and screen
reader affordance for links with `target` set to to `_blank`
([elastic#7352](elastic/eui#7352))
- Updated `EuiListGroupItem` with a new `external` prop, which allows
enabling or disabling the new external link icon
([elastic#7352](elastic/eui#7352))
- Updated `EuiText` to no longer set any opinionated styles on child
`<img>` tags - use `EuiImage` for image display within text instead
([elastic#7360](elastic/eui#7360))
- Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom
actions ([elastic#7361](elastic/eui#7361))
- Added a new `EuiDataGridToolbarControl` subcomponent, which is useful
for rendering your own custom `EuiDataGrid` toolbar buttons while
matching the look of the default controls
([elastic#7369](elastic/eui#7369))
- Updated `EuiDataGrid`'s toolbar controls to show active/current counts
in badges, and updated the Columns button icon
([elastic#7369](elastic/eui#7369))
- Updated `EuiButtonEmpty` to allow passing `false` to `textProps`,
which allows rendering custom button content without an extra text
wrapper ([elastic#7369](elastic/eui#7369))
- Updated `EuiDataGrid` column header cells to show the sort arrow after
the heading text, instead of before
([elastic#7371](elastic/eui#7371))
- Updated `EuiDataGrid`'s column header actions icon from a chevron to
`boxesVertical` ([elastic#7371](elastic/eui#7371))
- Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s.
Alongside `name`, the `description`, `href`, and `data-test-subj`
properties now also accept an optional callback that the current `item`
will be passed to ([elastic#7373](elastic/eui#7373))
- Updated `EuiContextMenuItem` with a new `toolTipProps` prop
([elastic#7373](elastic/eui#7373))
- `EuiSelectable` now allows configurable text truncation via
`listProps.truncationProps`
([elastic#7388](elastic/eui#7388))
- `EuiTextTruncate` now supports a new `calculationDelayMs` prop for
working around font loading or layout shifting scenarios
([elastic#7388](elastic/eui#7388))

**Bug fixes**

- Fixed incorrect `EuiPopover` positioning calculations when `hasArrow`
was set to false ([elastic#7343](elastic/eui#7343))
- Fixed `EuiSuperSelect` to render options with falsy values (false, 0,
and ''), but not nullish values (undefined or null)
([elastic#7362](elastic/eui#7362))
- Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g.,
booleans or numbers) ([elastic#7362](elastic/eui#7362))
- Fixed `EuiDataGrid`'s numeric and currency column heading cells to be
correctly right-aligned
([elastic#7371](elastic/eui#7371))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing
tooltip descriptions when rendered in the all actions popover menu
([elastic#7373](elastic/eui#7373))
- Fixed missing underlines on `EuiContextMenu` link hover
([elastic#7373](elastic/eui#7373))
- Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent`
([elastic#7375](elastic/eui#7375))
- Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off
in vertical alignment
([elastic#7380](elastic/eui#7380))

**Deprecations**

- Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use
`toolTipProps.title` instead
([elastic#7373](elastic/eui#7373))
- Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use
`toolTipProps.position` instead
([elastic#7373](elastic/eui#7373))

**Accessibility**

- Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested
interactive custom actions
([elastic#7361](elastic/eui#7361))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly
reading out action descriptions to screen readers
([elastic#7373](elastic/eui#7373))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not
visibly appearing on keyboard focus
([elastic#7373](elastic/eui#7373))

---------

Co-authored-by: Julia Rechkunova <[email protected]>
semd added a commit that referenced this pull request Feb 21, 2024
## Summary

issue: #177410

Adding `overflow: hidden` style was causing the cell actions to
disappear after the upgrade to [EUI upgrade to
v91.3.1](#173569)

The `overflow: hidden` style is no longer needed, it has been removed.
So cell actions are displayed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 21, 2024
## Summary

issue: elastic#177410

Adding `overflow: hidden` style was causing the cell actions to
disappear after the upgrade to [EUI upgrade to
v91.3.1](elastic#173569)

The `overflow: hidden` style is no longer needed, it has been removed.
So cell actions are displayed

(cherry picked from commit ab864b4)
kibanamachine referenced this pull request Feb 21, 2024
#177501)

# Backport

This will backport the following commits from `main` to `8.13`:
- [[Security Solution] Fix cell actions in Explore pages
(#177478)](#177478)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Sergi
Massaneda","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-02-21T17:52:45Z","message":"[Security
Solution] Fix cell actions in Explore pages (#177478)\n\n##
Summary\r\n\r\nissue:
https://github.com/elastic/kibana/issues/177410\r\n\r\nAdding `overflow:
hidden` style was causing the cell actions to\r\ndisappear after the
upgrade to [EUI upgrade
to\r\nv91.3.1](https://github.com/elastic/kibana/pull/173569)\r\n\r\nThe
`overflow: hidden` style is no longer needed, it has been removed.\r\nSo
cell actions are
displayed","sha":"ab864b4ccc699cc1a986bf099460ad877a42333d","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Threat
Hunting","Team:Threat
Hunting:Explore","v8.13.0","v8.14.0"],"title":"[Security Solution] Fix
cell actions in Explore
pages","number":177478,"url":"https://github.com/elastic/kibana/pull/177478","mergeCommit":{"message":"[Security
Solution] Fix cell actions in Explore pages (#177478)\n\n##
Summary\r\n\r\nissue:
https://github.com/elastic/kibana/issues/177410\r\n\r\nAdding `overflow:
hidden` style was causing the cell actions to\r\ndisappear after the
upgrade to [EUI upgrade
to\r\nv91.3.1](https://github.com/elastic/kibana/pull/173569)\r\n\r\nThe
`overflow: hidden` style is no longer needed, it has been removed.\r\nSo
cell actions are
displayed","sha":"ab864b4ccc699cc1a986bf099460ad877a42333d"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","branchLabelMappingKey":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/177478","number":177478,"mergeCommit":{"message":"[Security
Solution] Fix cell actions in Explore pages (#177478)\n\n##
Summary\r\n\r\nissue:
https://github.com/elastic/kibana/issues/177410\r\n\r\nAdding `overflow:
hidden` style was causing the cell actions to\r\ndisappear after the
upgrade to [EUI upgrade
to\r\nv91.3.1](https://github.com/elastic/kibana/pull/173569)\r\n\r\nThe
`overflow: hidden` style is no longer needed, it has been removed.\r\nSo
cell actions are
displayed","sha":"ab864b4ccc699cc1a986bf099460ad877a42333d"}}]}]
BACKPORT-->

Co-authored-by: Sergi Massaneda <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
## Summary

issue: elastic#177410

Adding `overflow: hidden` style was causing the cell actions to
disappear after the upgrade to [EUI upgrade to
v91.3.1](elastic#173569)

The `overflow: hidden` style is no longer needed, it has been removed.
So cell actions are displayed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting EUI release_note:skip Skip the PR/issue when compiling release notes v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.