-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 v43.1.1 #120935
Upgrade EUI to v43.1.1 #120935
Conversation
return { | ||
colorMode: darkMode ? 'DARK' : 'LIGHT', | ||
colorMode: darkMode ? COLOR_MODES_STANDARD.dark : COLOR_MODES_STANDARD.light, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks for that
* @internal Only meant to be used within core for internal usages of EUI/React | ||
*/ | ||
export const CoreThemeProvider: FC<CoreThemeProviderProps> = ({ theme$, children }) => { | ||
const coreTheme = useObservable(theme$, defaultTheme); | ||
const euiTheme = useMemo(() => convertCoreTheme(coreTheme), [coreTheme]); | ||
return ( | ||
<EuiThemeProvider colorMode={euiTheme.colorMode} theme={euiTheme.euiThemeSystem}> | ||
<EuiProvider | ||
modify={{ LIGHT: { colors: { primary: '#FF0000' } } }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think declaring a const for this modify
value outside of the component is slightly better for memory and rerendering triggers as it would not be recreated at each rerendering + only one instance will be used for all usages of CoreThemeProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flagging this. This was meant entirely to test the setup and will be removed before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will want to expand the API to pass modify
to KibanaThemeProvider, but I don't see that as an immediate need.
const emotionCache = createCache({ | ||
key: 'eui-styles', | ||
container: document.querySelector(`meta[name="${EUI_STYLES_GLOBAL}"]`) as HTMLElement, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, shouldn't this also be used in the public version of the EuiProvider wrapper in src/plugins/kibana_react/public/theme/kibana_theme_provider.tsx
? (same for modify
)
If that's the case, we'll have to expose this cache from core
somehow, so that both version of the wrapper use the same cache instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the top-level theme provider needs the cache because it's the only provider that now also provides global styles. Those global styles only need to be included once and are the only styles that require the cache. Eventually, once EUI has converted all components to Emotion, the global style cache will be unnecessary and can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, Just to be sure we're on the same page, this CoreThemeProvider
component is not used only once.
It is used as a 'top level' provider for chrome's react tree, but is also used for internal core's components, such as toasts, flyout and modal, as all those components are their own distinct react tree / ReactDOM.render call.
Is it still fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is fine, thanks for clarifying. My main point was that a nested KibanaThemeProvider does not need to be aware of the cache.
I'll probably have a follow-up PR that ensures no duplication of the global styles from the various toasts, flyout and modal, etc. instances, but there will be no adverse affects in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security solution changes LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data discover code changes LGTM, didn't test for now, but I'm excited that data grid row height selection is now available in Kibana 🥳
const toggleButtonChecked = await this.testSubjects.getAttribute( | ||
'superDatePickerToggleRefreshButton', | ||
'aria-checked' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I <3 this tweak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observability test changes look good, thanks for the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asset management LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML/transform changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no visible breakage in infra
and monitoring
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for core
changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App services LGTM
I tested refresh interval in Discover and Dashboard and didn't find any issues
@elasticmachine merge upstream |
Remaining reviews are for snapshot changes only. Will merge on the next successful CI run. |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* eui to v43.0.0 * update themeprovider types * update sass import location * storyshots * snapshots * jest * jest * timepicker functional test helpers * snapshots * timepicker functional test helpers * invert checked logic * fix i18n token * snapshots * handle new refresh interval operations order * refactor refresh interval logic, test * jest * timepicker popover order * eui to v43.1.0 * jest * jest * jest * euicode selector * jest * functional tests * functional tests * timepicker * bump timeout * conditional click to close * clean up * eui to v43.1.1 * clean up * loading background Co-authored-by: Kibana Machine <[email protected]>
This reverts commit 604409b.
Summary
[email protected]
⏩[email protected]
EuiCode
andEuiCodeBlock
have be refactored to share less code and simplify markupCoreThemeProvider
to useEuiProvider
, which extendsEuiThemeProvider
@emotion/cache
configuration for reset and global styles@emotion/cache
configuration for reset and global styles to ensure they load before static CSSshowStyleSelector
prop was renamedshowDisplaySelector
inEuiDataGrid
43.1.1
Bug fixes
EuiDataGrid
's cell popover overlapping with modals and flyouts (#5461)EuiDatePicker
time options did not have unique IDs (#5466)EuiSuperDatePicker
not passingisDisabled
toEuiAutoRefresh
(#5472)43.1.0
magnifyWithExclamation
icon (#5455)Bug fixes
EuiCode
andEuiCodeBlock
testenv
mocking (#5464)43.0.0
EuiDataGrid
's toolbar/grid controls (#5334)EuiDataGrid
's full screen mode to use thefullScreenExit
icon (#5415)left.append
andleft.prepend
toEuiDataGrid
'stoolbarVisibility.additionalControls
prop #5394)EuiDataGrid
's toolbar (#5372)onChange
callbacks toEuiDataGrid
'sgridStyle
androwHeightOptions
settings (#5424)EuiDataGrid
's display controls (#5428)timeRefresh
icon (#5383)responsive
andiconOnly
props toEuiSuperUpdateButton
(#5383)EuiSuperDatePicker
(#5383)compressed
,width
,isQuickSelectOnly
props toEuiSuperDatePicker
(#5383)showUpdateButton
prop withiconOnly
option inEuiSuperDatePicker
(#5383)refreshInterval
ofEuiSuperDatePicker
to1000
(#5383)Show dates
button from pretty format ofEuiSuperDatePicker
in favor of directly openingstart
date popover (#5383)shortHand
option toprettyInterval
(#5383)rounding
switch to popover footer in relative tab ofEuiSuperDatePicker
(#5383)EuiRefreshInterval
to use a switch to start/stop and other visual touch ups (#5383)EuiAutoRefresh
andEuiAutoRefreshButton
components (#5383)Bug fixes
EuiDataGrid
full screen<body>
class (#5354)EuiFormControlLayout
prepend
andappend
(#5383)EuiFormControlLayout
whenreadOnly
(#5383)data-test-subj
prop ofEuiFormControlLayout
(#5383)<button>
s (#5452)Breaking changes
toolbarVisibility
'sshowStyleSelector
prop ofEuiDataGrid
in favor ofshowDisplaySelector
, which allows configuration of both grid density and row height (#5372)applyRefreshInterval
toonRefreshChange
inEuiRefreshInterval
(#5383)s
-sizedEuiLoadingSpinner
s to matchs
-sizedEuiIcon
s (#5440)42.1.0
EuiPagination
whencompressed=true
(#5362)EuiPagination
whenpageCount=0
(#5362)responsive
prop toEuiPagination
that renders thecompressed
display (#5362)doubleArrowLeft
,doubleArrowRight
,arrowStart
,arrowEnd
icons (#5362)Bug fixes
EuiRange
tick labels in Safari (#5427)EuiOverlayMask
bug where it calls window.document on server side(#5422)EuiPopover
(#5437)EuiDatePicker
not correctly handling theonBlur
callback (#5441)EuiToolTip
not correctly handling childonBlur
andonFocus
callbacks (#5441)42.0.0
Feature: CSS-in-JS (#5121)
@emotion/react/Global
EuiProvider
, a React context provider for theming and global stylesisDefaultTheme
andisLegacyTheme
utilitiesBreaking changes
@emotion/react
topeerDependencies
globals
are imported from41.4.0
payment
glyph toEuiIcon
(#5414)EuiCodeBlock
's full screen mode to use thefullScreenExit
icon (#5421)Bug fixes
EuiCodeBlock
bug where empty code blocks could be copyable (#5421)EuiAvatar
by adding a meaningful role (#5423)41.3.0
EuiHorizontalRule
when rendered insideEuiToolTip
(#5378)Bug fixes
EuiDataGrid
bug where paginated overflowing data grids could become unscrollable whenrowCount
changed (#5400)EuiCode
line-wrapping (#5379)EuiCodeBlock
not passingdata-test-subj
oraria-label
to virtualized & full-screen code blocks (#5379)EuiCodeBlock
not closing full-screen mode when the Escape key is pressed (#5379)EuiCodeBlock
s blanking out when entering & exiting full-screen mode (#5379)EuiCodeBlock
's full-screen mode to use a large font and padding size & added several missing wrapper classes (#5379)EuiCodeBlock
broken line wrapping when using virtualization (#5379)Theme: Amsterdam
EuiCodeBlock
not properly increasing large font sizes on Amsterdam (#5379)For maintainers