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 v93.6.0 #180316

Merged
merged 11 commits into from
Apr 9, 2024
Merged

Upgrade EUI to v93.6.0 #180316

merged 11 commits into from
Apr 9, 2024

Conversation

cee-chen
Copy link
Contributor

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

v93.5.2v93.6.0


v93.6.0

  • Updated EuiBreadcrumb styles to improve visual distinction of clickable breadcrumbs (#7615)

Deprecations

  • Deprecated color prop on EuiBreadcrumb (#7615)

Bug fixes

  • Fixed EuiComboBox to correctly select full matches within groups via the Enter key (#7658)

Accessibility

  • Updated EuiHeaderBreadcrumb styles to ensure min. required color contrast (#7643)
  • EuiSuperSelect now correctly reads out parent EuiFormRow labels to screen readers (#7650)
  • EuiSuperSelect now more closely mimics native <select> behavior in its keyboard behavior and navigation (#7650)
  • EuiSuperSelect no longer strands keyboard focus on close (#7650)
  • EuiSuperSelect now correctly allows keyboard navigating past disabled options in the middle of the options list (#7650)

@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes EUI v8.14.0 labels Apr 8, 2024
cee-chen added 6 commits April 8, 2024 12:00
- removed unnecessary modifier classes
- `-isInteractive` appended to account for new styles
…der behavior

A visually hidden `, ` is now included in the selected option text for super select components with used within `<EuiFormRow>`s with labels

This affects `===` text assertions, but changing them to `contains` instead if a simpler and quicker approach that keeps the meaning of the test while passing it
@cee-chen
Copy link
Contributor Author

cee-chen commented Apr 8, 2024

/ci

@cee-chen cee-chen marked this pull request as ready for review April 8, 2024 22:26
@cee-chen cee-chen requested review from a team as code owners April 8, 2024 22:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-team (EUI)

@cee-chen cee-chen requested review from a team as code owners April 8, 2024 22:26
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Apr 8, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Vis changes LGTM 👍🏼

@cee-chen
Copy link
Contributor Author

cee-chen commented Apr 9, 2024

👋 Hey y'all! As this PR fairly minor and is 90% snapshot updates (and here is your scheduled reminder to consider moving away from snapshots!), we'll be asking KibanaOps for an admin merge (regardless of approvals) by Wednesday.

We have a much larger release & upgrade PR landing very shortly that will contain EuiTable (including basic and in memory table) breaking changes and enhancements, and we'd much rather y'all spend your time reviewing and QAing that one. Stay tuned!

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Fleet changes LGTM

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.

@elastic/security-defend-workflows related changes LGTM

one minor thing as a paranoid test writer: we had changes only in test files, all of them are around changing the assertion from toEqual() to toContain(), because the rendered text content now ends with , .

do you think this change could decrease trust in tests? e.g. for OSes it asserts it contains 'windows', but in the background the element contains all the other OSes as well because of e.g. a bad selector or a future change in EUI or a future bug introduced by a developer in our team.

if you're confident with these changes, I'm okay with them, otherwise I'd suggest going with toEqual('Windows, ')

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

Obs ux LGTM

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.

Vis changes lgtm, (tested the combobox)

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Fleet changes 🚀

Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Snapshot changes in es_ui_shared plugin LGTM

@cee-chen
Copy link
Contributor Author

cee-chen commented Apr 9, 2024

@gergoabraham

one minor thing as a paranoid test writer: we had changes only in test files, all of them are around changing the assertion from toEqual() to toContain(), because the rendered text content now ends with , .

do you think this change could decrease trust in tests? e.g. for OSes it asserts it contains 'windows', but in the background the element contains all the other OSes as well because of e.g. a bad selector or a future change in EUI or a future bug introduced by a developer in our team.

if you're confident with these changes, I'm okay with them, otherwise I'd suggest going with toEqual('Windows, ')

In this case with the tests as written, I'm fairly confident for two reasons: EuiSuperSelect can only ever contain 1 selection and not multiple, and the options in the test (Windows, Mac, and Linux) are different enough that the wrong item being selected will correctly trigger a failure.

That being said, I totally get it if your team would prefer a more strict assertion, and that's totally valid. I'll go ahead and make that change for your team's test files now!

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

AppEx-SharedUX changes LGTM

Reviewed the snapshot changes.

@cee-chen cee-chen enabled auto-merge (squash) April 9, 2024 16:48
@cee-chen cee-chen merged commit 459ed57 into elastic:main Apr 9, 2024
38 checks passed
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id before after diff
core 405.8KB 405.7KB -94.0B
kbnUiSharedDeps-npmDll 6.3MB 6.3MB +2.6KB
total +2.5KB

History

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

@cee-chen cee-chen deleted the eui/v93.6.0 branch April 9, 2024 21:49
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 9, 2024
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 Team:Fleet Team label for Observability Data Collection Fleet team v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.