Deprecate static breakpoint services #6119
Conversation
- in favor of _EuiThemeBreakpoints type from global_styling
- Preserve the type export, since the global_styling one has a _ prefix, but move the export out to the main index.ts
+ rename hook file to more generic name in prepration for max/min width hooks + use snake_case to match rest of repo
+ add tests that previously did not exist
cee-chen
force-pushed
the
breakpoints-deprecations
branch
from
d13a5a1
to
c36b7f9
Compare
+ per new hook typing, only accept a named size instead of a number
- in favor of memoizing its behavior in `CurrentEuiBreakpoint` logic + update testenv mock slightly - it's not super DRY, but it's fine
cee-chen
force-pushed
the
breakpoints-deprecations
branch
from
c36b7f9
to
dc294a6
Compare
| * Named breakpoint (`xs` through `xl`) for customizing the minimum window width to enable the `push` type | ||
| */ | ||
| pushMinBreakpoint?: EuiBreakpointSize | number; | ||
| pushMinBreakpoint?: EuiBreakpointSize; |
There was a problem hiding this comment.
Definitely open to thoughts on this change, but Kibana doesn't appear to be using either pushMinBreakpoint or dockedBreakpoint and I thought it would be simpler (especially now that we allow overriding theme breakpoints/adding new breakpoints) to only accept named sizes instead of arbitrary numerical widths.
| // Ensure the breakpoints map is sorted from largest value to smallest | ||
| const sortedBreakpoints: _EuiThemeBreakpoints = useMemo( | ||
| () => sortMapByLargeToSmallValues(breakpoints), | ||
| [breakpoints] | ||
| ); | ||
|
|
||
| // Find the breakpoint (key) whose value is <= windowWidth starting with largest first | ||
| const getBreakpoint = useCallback( | ||
| (width: number) => | ||
| keysOf(sortedBreakpoints).find((key) => sortedBreakpoints[key] <= width), | ||
| [sortedBreakpoints] | ||
| ); |
There was a problem hiding this comment.
(The above code follows up on the memoization comment I made in #6111 (comment))
I went back and forth on creating a new useGetBreakpoint() hook vs simply baking getBreakpoint into the CurrentEuiBreakpoint logic.
After looking more closely at Kibana's usage of getBreakpoint, I decided on deprecating in favor of useCurrentEuiBreakpoint. Out of 5 usages of getBreakpoint, only 1 appears to be using a ResizeObserver instead of window width, and based on its code comments the EuiResizeObserver appears to be used as syntactical sugar and they primarily want to respond to window width. TL;DR, I'm 99% sure all usages of getBreakpoint in Kibana can be replaced with useCurrentEuiBreakpoint.
I also slightly preferred baking getBreakpoint's logic into the the provider because you cannot call a hook within a callback, which would have made useGetBreakpoint usage in the onWindowResize callback fairly tricky.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6119/ |
Summary
I strongly recommend following along by commit!
BREAKPOINTS,BREAKPOINT_KEYS,getBreakpoint,isWithinBreakpoints,isWithinMaxBreakpoint, andisWithinMinBreakpointbreakpoint servicesuseIsWithinMaxBreakpointanduseIsWithinMinBreakpointhooks. These hooks correctly respect EUI theme breakpoint overrides instead of using static Amsterdam defaults.isWithinMinBreakpoint/ repeating their own window resize listeners (EuiFlyout and EuiCollapsibleNav) to useuseIsWithinMinBreakpointScreenshots
Checklist
- [ ] Checked in both light and dark modes- [ ] Checked in Chrome, Safari, Edge, and Firefox- [ ] Props have proper autodocs and playground toggles- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart