-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Bumping EUI to version 64.0.4 #140323
Bumping EUI to version 64.0.4 #140323
Conversation
- in favor of useCurrentEuiBreakpoint
- in favor of hook
…e resize observer - since getBreakpoint is now deprecated, we're only looking at the current EUI breakpoint (based off window width)
- Setup for custom xl+ breakpoint sizes used by APM and Synthetics
- since getBreakpoint is no longer an usable util - EUI's new breakpoint hooks already use/debounce window resize events, so we now get to skip all that custom logic by simply making custom EUI breakpoint overrides and using EUI breakpoint hooks - remove returned `breakpoint` and `width` keys - they weren't actually being used anywhere in APM that I could see and were causing type errors. - If someone wants to access them, they can use `useCurrentEuiBreakpoint` and `useWindowSize` individually instead
- basically the exact same logic, they just also need the xxl and xxxl breakpoints
…ooks + add xxl and xxxl breakpoints to Synthetics EUI themes - `useBreakpoints`: - as far as I can tell, all attached APIs are basically the same APIs that EUI provides OOTB: - up -> `useIsWithinMinBreakpoint` - down -> `useIsWithinMaxBreakpoint` - between -> `useIsWithinBreakpoints` - debouncedWidth - not used, but could just use `useWindowSize` directly instead - note: i'm confused by the `xl` override/conflation with `xxl`, but left the default xl breakpoint size as-is and assumed that all usage instances of 'xl' actually wanted 'xxl'
- functionally doesn't matter since the array doesn't include undefined, so basically just silence TS complaining about it
… vs table resize observer" This reverts commit 4e60ce2.
- by replacing the breakpoint logic/switch with custom breakpoint sizes and names - since the table size can be within panels that don't match `window.innerWidth`, we can't use EUI's breakpoint utils here
- because of their new custom xxl/xxxl breakpoints, the `isLargerBreakpoint` logic was broken on the shared solution nav. switching to a min breakpoint bound fixes the issue and futureproofs the solution nav for all apps that add larger breakpoints than xl
Pinging @elastic/uptime (Team:uptime) |
Pinging @elastic/fleet (Team:Fleet) |
Pinging @elastic/apm-ui (Team:APM) |
👋 Hey y'all! The major breaking change of this EUI upgrade is the removal of all our non-hook breakpoint utils. Because breakpoints can now be customized through This most affects the following teams/plugins:
For all of the above mentioned teams, I added 2 new breakpoints to your apps/EUI themes: Other teams that received mostly 1:1 / non-major hook conversions were:
If you see any issues or bugs behavior around the new breakpoint logic/UX, please don't hesitate to reach out or ping us! |
@@ -100,7 +101,7 @@ export const SolutionNav: FC<SolutionNavProps> = ({ | |||
}) => { | |||
const isSmallerBreakpoint = useIsWithinBreakpoints(mobileBreakpoints); | |||
const isMediumBreakpoint = useIsWithinBreakpoints(['m']); | |||
const isLargerBreakpoint = useIsWithinBreakpoints(['l', 'xl']); | |||
const isLargerBreakpoint = useIsWithinMinBreakpoint('l'); |
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.
what does this do? and where did xl
go?
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.
With the refactored breakpoint hooks, we discovered a situation where the side nav would stop rendering if the browser window was 1600px or wider. By switching to the useIsWithinMinBreakpoint('l')
we removed that upper range and effectively said at large breakpoint or wider, always evaluate to true.
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.
commit message reference: cf858f1
because of [apm/ux/uptime/synthetic's] new custom xxl/xxxl breakpoints, the
isLargerBreakpoint
logic was broken on the shared solution nav. switching to a min breakpoint bound fixes the issue and futureproofs the solution nav for all apps that add larger breakpoints than xl
For more context: EUI now supports both customizing our existing breakpoint sizes (xs
through xl
) and adding more custom breakpoints (in this case, xxl: 1600
and xxxl: 2000
for the apm/ux/uptime/synthetic plugins).
This means that using useIsWithinBreakpoints['l', 'xl']
is unintentionally too limiting for plugins with custom larger breakpoints, it translates to is within the l and xl screens only
whereas this logic really wants is at least the l screen and above
, which is what useIsWithinMinBreakpoint
does.
@@ -5,7 +5,6 @@ | |||
* 2.0. | |||
*/ | |||
|
|||
import { getBreakpoint } from '@elastic/eui'; |
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.
Is it possible to make use of useCurrentEuiBreakpoint
here so we don't have to use hard coded widths like (width <= 575)
further down?
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.
Unfortunately, no. I tried it in 4e60ce2, but this caused several functional tests to fail (correctly so). I had to revert the commit and do 2aa2234 instead.
The reason why is because useCurrentEuiBreakpoint
only uses window.innerWidth
and does not accept variable widths like getBreakpoint
. Unfortunately, DataVisualizerTable
is not always the width of the screen; it is occasionally in panels that are (e.g.) half the size of the screen or less.
So this logic does not translate to the new breakpoint hooks; but to be honest the concept of a breakpoint does tie in to windows/screens and not necessarily arbitrary component sizes/container queries, so the switch usage is perfectly valid - it just happens to use widths that correspond to EUI's breakpoint sizes (which you can now easily change to tailor to your component).
Hope that makes sense, if not LMK and I can tag you into a Slack thread that had more discussion on this.
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 clarifying! We'll pick it up in a follow up. Can you please just add a comment like // TODO migrate to use useCurrentEuiBreakpoint
or similar in the section below with the hard coded values?
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'm not sure I explained well. You cannot migrate your current resize observer / calculateTableColumnsDimensions
logic to useCurrentEuiBreakpoint
because your table width is not always 1:1 with window.innerWidth
.
At most you can move away the static numbers in your branching logic to not be 1:1 with EUI's breakpoint sizes so it's clear you're not trying to use our screen size breakpoints.
cc @qn895 on this as we discussed this while I was working on the deprecation, hoping you can help clarify!
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.
Quynh pinged me about it and I suggested this: e6be719
Using a static map/constant and changing the breakpoints slightly from EUI may affect functionality slightly compared to before but helps more clearly distinguish that this table uses its own component-width-based breakpoints, and not EUI's screen-size-based breakpoints.
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.
src/plugins/kibana_utils/public/theme/kibana_theme_provider.tsx
lgtm
@elasticmachine merge upstream |
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 changes LGTM, we'll pick up using the new breakpoint utils in a follow up.
@@ -5,7 +5,6 @@ | |||
* 2.0. | |||
*/ | |||
|
|||
import { getBreakpoint } from '@elastic/eui'; |
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 clarifying! We'll pick it up in a follow up. Can you please just add a comment like // TODO migrate to use useCurrentEuiBreakpoint
or similar in the section below with the hard coded values?
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.
shared-ux
changes look good; code review only
…kpoints - per discussion with Quynh and Walter, there's some confusion around the breakpoints that the table is using. Since the table is width is not always the width of the browser window, it cannot use EUI's breakpoint hooks, and should instead use its own custom map of table-specific breakpoints to make that separation clearer
…-ref HEAD~1..HEAD --fix'
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @1Copenut |
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.
APM UI 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.
LGTM! Also tested the synthetics app E2E for these changes and it seems to be working just fine. Thanks for the update.
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.
Looks good! Thank you 😊
Summary
eui@63.0.6
⏩eui@64.0.4
64.0.4
Note: this release is a backport containing changes originally made in
67.2.0
custom_component
search filter type for the EuiSearchBar. This new type gives the consumer control to render the search filter dropdown. (#6226)Bug fixes
EuiPageSection
not correctly mergingcontentProps.css
(#6239)EuiPageHeaderContent
not correctly merging passedclassName
s (#6239)EuiAccordion
not correctly mergingbuttonProps.css
andarrowProps.css
(#6239)EuiProgress
not correctly merginglabelProps.css
(#6239)EuiImage
not correctly mergingwrapperProps.css
(#6239)64.0.3
Note: this release is a backport containing changes originally made in
66.0.0
Bug fixes
EuiHeaderSectionItem
to not render if empty (#6158)64.0.2
Note: this release is a backport containing changes originally made in
65.0.0
to67.0.0
Bug fixes
EuiDataGrid
cell popover shadows in Safari (#6163)useEuiTheme
's return value, supporting React's shallow prop comparison optimizations (#6165)EuiProvider
cache is not configured. (#6202)componentProps
throwing type errors on thecss
prop (#6211)Note: The below are backport changes already present in
63.x
component
prop toEuiPageSection
, allowing overriding of the defaultsection
tag (#6192)Bug fixes
EuiDescriptionListTitle
whenEuiDescriptionList
is compressed (#6160)EuiPageSidebar
bug where inline styles were not correctly updating (#6191)Deprecations
@deprecated
flags toEuiPageContent_Deprecated
,EuiPageContentBody_Deprecated
,EuiPageContentHeader_Deprecated
,EuiPageContentHeaderSection_Deprecated
,EuiPageSideBar_Deprecated
andEuiPageTemplate_Deprecated
, which will provide helpful hints to IDEs that support jsdoc flags. Consumers will have until August 2023 to migrate from these deprecated components. (#6194)64.0.1
Bug fixes
CollapsedItemActions
ref callback not accounting fornull
value (#6145)CSS-in-JS conversions
logicalCSSWithFallback()
utility for logical properties without full browser support (#6124)euiFullHeight()
Sass mixin to Emotion (#6124)64.0.0
onPositionChange
callback prop toEuiPopover
for when the popover positon changes (#6087)isDisabled
prop toEuiAccordion
(#6095)css
prop toCommonProps
interface (#6118)useIsWithinMaxBreakpoint
anduseIsWithinMinBreakpoint
service hooks (#6119)Bug fixes
steps
prop type foruseEuiTour
to not requireonFinish
(#6087)useCurrentEuiBreakpoint
,useIsWithinBreakpoints
, andeuiBreakpoint
) to correctly handle custom theme breakpoint keys (#6111):first-child/:nth-child
console warnings for consumers not passing in acache
toEuiProvider
(#6126)EuiScreenReaderLive
double announcements on VO whenfocusRegionOnTextChange
is not set (#6133)onBlur
andonFocus
handlers fromEuiDatePickerRange
being incorrectly applied to wrapping element rather than the start/end control datepickers. (#6136)data-fixed-headers
property in some layout configurations usingEuiPageTemplate
. (#6140)EuiAspectRatio
sometimes incorrectly inheriting its height from parent containers as opposed to from its aspect ratio (#6141)EuiAspectRatio
to allow customstyle
s to be passed by consumers (#6141)eui.d.ts
containing@testing-library
type definitions (#6142)Breaking changes
getBreakpoint
. UseuseCurrentEuiBreakpoint
instead (#6119)BREAKPOINTS
andBREAKPOINT_KEYS
. UseeuiTheme.breakpoint
instead (#6119)isWithinBreakpoints
. UseuseIsWithinBreakpoints
instead (#6119)isWithinMaxBreakpoint
. UseuseIsWithinMaxBreakpoint
instead (#6119)isWithinMinBreakpoint
. UseuseIsWithinMinBreakpoint
instead (#6119)EuiFlyout
now only accepts a named breakpoint size for itspushMinBreakpoint
prop (#6119)EuiCollapsibleNav
now only accepts a named breakpoint size for itsdockedBreakpoint
prop (#6119)@emotion/cache
is now a required peer dependency, alongside@emotion/react
(#6126)CSS-in-JS conversions
EuiTour
to Emotion (#6087)