Create new `useCurrentEuiBreakpoint()` utility & provider #6079

cee-chen · 2022-07-26T23:22:55Z

Summary

I strongly recommend following along by commit.

What this PR does

This PR DRYs out many repeated window.addEventListener('resize') events across our codebase (which are all creating resize listeners for each instance of each component) in favor of a single resize event listener that returns the current window width as a named EUI breakpoint size (xs through xl).

In order to make this single resize listener possible, a shared context was created and nested within the <EuiProvider> component. bbac89d

Unlike our current breakpoint services / vanilla JS utility fns, this new hook (and now useIsWithinBreakpoints, which dogfoods it) correctly inherits any theme breakpoints override passed to EuiProvider. 🎉

What this PR does NOT do

This PR deliberately avoids breaking changes. As a result, I avoided the following:

This PR does NOT touch getBreakpoint, isWithinBreakpoints, isWithinMaxBreakpoint, or isWithinMinBreakpoint to fix/update them to respect theme breakpoints/overrides
This PR does NOT DRY out EuiCollapsibleNav or EuiFlyout, as they use isWithinMinBreakpoint

I'm hoping to get more feedback / discussion on how to tackle the above 2 problems and then open up a follow-up PR.

Personally, my strong preference would be to deprecate/remove the 4 exported vanilla JS services completely (possibly in favor of hooks that can call useEuiTheme() to properly get the current theme) and also deprecate the option for them to take static width numbers and take sizes only. Thoughts?

Checklist

Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Added documentation
Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately
A changelog entry exists and is marked appropriately

~~- [ ] Checked in both light and dark modes~~
- [ ] 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~~

- to allow creating a single provider and splitting up utils

- instead of src/services/hooks - see src/services/accessibility for another example of a non-generic hook that should live within a specific topic folder

… the EuiProvider level

… current breakpoint util

- without a callback, closeMenu reference will be lost and will not be correctly removed

- already declared in our global_styling / themes

…docs - since those utils do not correct inherit theme overrides, we should likely be deprecating and preferring hooks for future usage

…uirement

cee-chen · 2022-07-26T23:43:15Z

src/services/breakpoint/currentEuiBreakpoint.tsx

+    const onWindowResize = throttle(() => {
+      setCurrentBreakpoint(getBreakpoint(window.innerWidth, breakpoints));
+    }, 50);


[Perf question] Some of our components used a throttle ms of 50 and some didn't. Since going down to a single (per EuiProvider) resize listener is already more performant than what we had previously, do we have any opinions on whether we need to continue to optimize/throttle this resize listener?

If yay: should we also making this throttle number configurable by consumers, e.g. <EuiProvider breakpointResizeThrottle={0} >? 🤷

Let's keep it at 50 for now. I'm sure it was a fairly arbitrary choice that was propagated throughout, but I don't see a reason at the moment to make it configurable.

cee-chen · 2022-07-26T23:43:30Z

src/services/breakpoint/breakpoint.ts

@@ -41,7 +33,7 @@ export function getBreakpoint(
  breakpoints: EuiBreakpoints = BREAKPOINTS
 ): EuiBreakpointSize | undefined {
  // Find the breakpoint (key) whose value is <= windowWidth starting with largest first
-  return keysOf(BREAKPOINTS).find((key) => breakpoints[key] <= width);
+  return BREAKPOINT_KEYS.find((key) => breakpoints[key] <= width);
 }

 /**


This PR deliberately avoids breaking changes. As a result, I avoided touching services/breakpoint.ts's getBreakpoint, isWithinBreakpoints, isWithinMaxBreakpoint, or isWithinMinBreakpoint to fix or update them to respect theme breakpoints/overrides.

I'm hoping to get more feedback & discussion on how to tackle the above 2 problems and then open up a follow-up PR.

Personally, my strong preference would be to deprecate/remove the 4 exported vanilla JS services completely (possibly in favor of hooks that can call useEuiTheme() to properly get the current theme), and also deprecate the option for them to take static width numbers and take sizes only. Thoughts?

I avoided touching services/breakpoint.ts's getBreakpoint, isWithinBreakpoints, isWithinMaxBreakpoint, or isWithinMinBreakpoint

👍

deprecate/remove the 4 exported vanilla JS services completely

This seems like the right choice. Without access to the current theme variables they present liabilities.
getBreakpoint, isWithinMaxBreakpoint, isWithinMinBreakpointThere are used in Kibana but only to create a useBreakpoints hook, which could simply be replaced with something from EUI.

kibanamachine · 2022-07-26T23:56:32Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6079/

thompsongl

Nicely done, @constancecchen!

I'm going to run it through some manual testing in Kibana to check on any rerender side effects, but everything else is looking good.

thompsongl · 2022-07-27T14:41:39Z

wiki/consuming.md

  );
 };
 ```
+#### A not-recommended, legacy method to consume theming variables from Sass


🙇‍♂️

thompsongl · 2022-07-27T14:44:55Z

src/services/breakpoint/currentEuiBreakpoint.tsx

+    const onWindowResize = throttle(() => {
+      setCurrentBreakpoint(getBreakpoint(window.innerWidth, breakpoints));
+    }, 50);


Let's keep it at 50 for now. I'm sure it was a fairly arbitrary choice that was propagated throughout, but I don't see a reason at the moment to make it configurable.

thompsongl · 2022-07-27T14:49:25Z

src/services/breakpoint/breakpoint.ts

@@ -41,7 +33,7 @@ export function getBreakpoint(
  breakpoints: EuiBreakpoints = BREAKPOINTS
 ): EuiBreakpointSize | undefined {
  // Find the breakpoint (key) whose value is <= windowWidth starting with largest first
-  return keysOf(BREAKPOINTS).find((key) => breakpoints[key] <= width);
+  return BREAKPOINT_KEYS.find((key) => breakpoints[key] <= width);
 }

 /**


I avoided touching services/breakpoint.ts's getBreakpoint, isWithinBreakpoints, isWithinMaxBreakpoint, or isWithinMinBreakpoint

👍

deprecate/remove the 4 exported vanilla JS services completely

This seems like the right choice. Without access to the current theme variables they present liabilities.
getBreakpoint, isWithinMaxBreakpoint, isWithinMinBreakpointThere are used in Kibana but only to create a useBreakpoints hook, which could simply be replaced with something from EUI.

thompsongl

I'm going to run it through some manual testing in Kibana to check on any rerender side effects

LGTM!

cee-chen · 2022-07-27T20:16:42Z

Woohoo, thanks Greg!

cee-chen added 11 commits July 26, 2022 14:40

[Setup] Move src/services/breakpoint to folder

82a67a1

- to allow creating a single provider and splitting up utils

[Organization] Move useIsWithinBreakpoints to src/services/breakpoints

8ee6706

- instead of src/services/hooks - see src/services/accessibility for another example of a non-generic hook that should live within a specific topic folder

Add single currentEuiBreakpoint context/resize listener and add it at…

bbac89d

… the EuiProvider level

Add Jest testenv mock for unit tests without a wrapping EuiProvider

fcffff5

DRY out useIsWithinBreakpoints, EuiHideFor, and EuiShowFor to use new…

dd0986f

… current breakpoint util

DRY out EuiBreadcrumbs and EuiSelectableTemplateSitewide to use new util

712ee8e

[Misc perf] Fix EuiHeaderLinks resize listener

df91815

- without a callback, closeMenu reference will be lost and will not be correctly removed

DRY out repeated breakpoint types/defs

f2e31fd

- already declared in our global_styling / themes

DRY out breakpoints docs resize listeners

149c841

[Docs] Document new useCurrentEuiBreakpoint hook and remove non-hook …

d87ee5f

…docs - since those utils do not correct inherit theme overrides, we should likely be deprecating and preferring hooks for future usage

Update wiki/consuming.md to more clearly indicate the EuiProvider req…

d0c4d76

…uirement

cee-chen requested a review from thompsongl July 26, 2022 23:22

Changelog

8d35397

cee-chen commented Jul 26, 2022

View reviewed changes

cee-chen marked this pull request as ready for review July 26, 2022 23:44

thompsongl reviewed Jul 27, 2022

View reviewed changes

thompsongl approved these changes Jul 27, 2022

View reviewed changes

cee-chen merged commit 85ca5fe into elastic:main Jul 27, 2022

cee-chen deleted the breakpoints branch July 27, 2022 20:16

thompsongl mentioned this pull request Aug 15, 2022

Upgrade EUI to v62.2.3 elastic/kibana#138837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new `useCurrentEuiBreakpoint()` utility & provider #6079

Create new `useCurrentEuiBreakpoint()` utility & provider #6079

cee-chen commented Jul 26, 2022 •

edited

Loading

cee-chen Jul 26, 2022 •

edited

Loading

thompsongl Jul 27, 2022

cee-chen Jul 26, 2022 •

edited

Loading

thompsongl Jul 27, 2022

kibanamachine commented Jul 26, 2022

thompsongl left a comment

thompsongl Jul 27, 2022

thompsongl Jul 27, 2022

thompsongl Jul 27, 2022

thompsongl left a comment

cee-chen commented Jul 27, 2022

Create new useCurrentEuiBreakpoint() utility & provider #6079

Create new useCurrentEuiBreakpoint() utility & provider #6079

Conversation

cee-chen commented Jul 26, 2022 • edited Loading

Summary

What this PR does

What this PR does NOT do

Checklist

cee-chen Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

thompsongl Jul 27, 2022

Choose a reason for hiding this comment

cee-chen Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

thompsongl Jul 27, 2022

Choose a reason for hiding this comment

kibanamachine commented Jul 26, 2022

thompsongl left a comment

Choose a reason for hiding this comment

thompsongl Jul 27, 2022

Choose a reason for hiding this comment

thompsongl Jul 27, 2022

Choose a reason for hiding this comment

thompsongl Jul 27, 2022

Choose a reason for hiding this comment

thompsongl left a comment

Choose a reason for hiding this comment

cee-chen commented Jul 27, 2022

Create new `useCurrentEuiBreakpoint()` utility & provider #6079

Create new `useCurrentEuiBreakpoint()` utility & provider #6079

cee-chen commented Jul 26, 2022 •

edited

Loading

cee-chen Jul 26, 2022 •

edited

Loading

cee-chen Jul 26, 2022 •

edited

Loading