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 v81.0.0 #158330

Merged
merged 20 commits into from
May 31, 2023
Merged

Upgrade EUI to v81.0.0 #158330

merged 20 commits into from
May 31, 2023

Conversation

1Copenut
Copy link
Contributor

Summary

@elastic/eui@80.0.0@elastic/eui@81.0.0


81.0.0

  • Added ability to set options.checked to "mixed" in EuiSelectable (#6774)

Bug fixes

  • Portalled components (e.g. EuiPopover, EuiModal, EuiFlyout) will correctly inherit text color from its nearest EuiThemeProvider parent. <EuiText color="default"> is no longer needed. (#6775)

Breaking changes

  • EuiSelectable no longer renders a data-test-selected attribute on its list items. Use the aria-checked property instead (#6774)
  • Nested EuiThemeProviders now render a wrapping <span> element in order to correctly set the inherited text color of all descendants. <EuiText color="default"> is no longer needed. (#6775)

@1Copenut 1Copenut added release_note:skip Skip the PR/issue when compiling release notes EUI backport:skip This commit does not require backporting v8.9.0 labels May 23, 2023
@1Copenut 1Copenut self-assigned this May 23, 2023
@1Copenut 1Copenut marked this pull request as ready for review May 26, 2023 20:24
@1Copenut 1Copenut requested review from a team as code owners May 26, 2023 20:24
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-team (EUI)

@1Copenut 1Copenut requested a review from jpdjere May 26, 2023 20:24
@pzl pzl requested a review from ashokaditya May 26, 2023 20:36
@walterra
Copy link
Contributor

I got a question regarding nested EuiThemeProviders (elastic/eui#6775). It seems adding the span elements will potentially end up with a nesting of block elements like divs within those inline span elements. Is that something you discussed, just wondering if you considered that sort of inline/block nesting fine for this case.

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Visualizations team changes LGTM

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

security_solution/public/management changes LGTM.

@cee-chen
Copy link
Contributor

cee-chen commented May 30, 2023

I got a question regarding nested EuiThemeProviders (elastic/eui#6775). It seems adding the span elements will potentially end up with a nesting of block elements like divs within those inline span elements. Is that something you discussed, just wondering if you considered that sort of inline/block nesting fine for this case.

Thanks for the super thoughtful and engaged question @walterra! The concept of "block" vs "inline" content no longer has any semantic meaning as of HTML5. It's now solely a presentational aspect of CSS (see MDN docs), and there is no longer any validation or concern around a block element nested within an inline wrapper.

In this case, I opted for an inline span element because I was trying to make the wrapper as unobtrusive as possible and as flexible for as many cases as possible. For block children, the inline span naturally wraps around them to become 100% width. For inline children (e.g. buttons), the span also wraps around them as naturally as possible without creating a new full-width context on the page.

That being said, yes, an escape hatch exists via the new <EuiThemeProvider wrapperProps> prop. If you specifically want your wrapper to display as a block, or flex, or grid, or what have you, you could do something like this:

<EuiThemeProvider wrapperProps={{ style: { display: 'block' } }}>

Or alternatively, if you're using the theme provider in a layout-sensitive context, e.g. as a flexbox or grid child, and the child of your EuiThemeProvider is a single element, you could use wrapperProps.cloneElement:

<EuiFlexGroup>
  <EuiThemeProvider colorMode="inverse" wrapperProps={{ cloneElement: true }}>
    <EuiFlexItem grow={false}> {/* only this wrapper will render */}
      // ...
    </EuiFlexItem>
  </EuiThemeProvider>
</EuiFlexGroup>

I'll do another pass here on this PR for usages of <EuiThemeProvider> to see if I can see in code where Kibana teams may need wrapperProps.cloneElement. Thanks for the reminder!

@cee-chen
Copy link
Contributor

@elasticmachine merge upstream

kibanamachine and others added 4 commits May 30, 2023 15:43
@cee-chen cee-chen requested a review from a team May 30, 2023 20:45
Copy link
Contributor

@cqliu1 cqliu1 left a comment

Choose a reason for hiding this comment

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

Presentation team changes LGTM 👍

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

ML related changes LGTM.

@cee-chen thanks for sharing all the details related to EuiThemeProvider, great to see this gives us all necessary flexibility for edge cases!

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Files owned by Defend Workloads LGTM 👍

@cee-chen
Copy link
Contributor

@elasticmachine merge upstream

@cee-chen cee-chen enabled auto-merge (squash) May 31, 2023 17:50
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeVega 292 305 +13

Async chunks

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

id before after diff
infra 2.0MB 2.0MB +1.7KB
securitySolution 9.2MB 9.2MB -57.0B
serverlessSearch 51.6KB 51.5KB -48.0B
visTypeVega 1.7MB 1.7MB +8.0KB
total +9.5KB

Page load bundle

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

id before after diff
core 365.8KB 366.2KB +407.0B
kbnUiSharedDeps-npmDll 5.9MB 6.0MB +2.9KB
visTypeVega 33.7KB 33.8KB +110.0B
total +3.4KB
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 416 420 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 500 504 +4
total +6

History

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

cc @1Copenut

@cee-chen cee-chen merged commit aa1d266 into elastic:main May 31, 2023
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.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.