[EuiCollapsibleNavBeta] Build out component state and responsive behavior #7027

cee-chen · 2023-08-02T00:37:29Z

Summary

⚠️ This PR is part 1 of 2 of completing EuiCollapsibleNavBeta's responsive/docked behavior.
- The part that is missing is the part where EuiCollapsibleNavItem needs to conditionally render a icon that toggles a popover, but only for larger desktop screens when in the "collapsed" state.
- I opted to split out this work into separate PRs, as this one was already getting pretty long in the tooth in terms of LOC/review time.
Parent issue: Updates to navigation components in support of Kibana nav redesign #6759
Previous PRs: [Beta] Add EuiCollapsibleNavItem component #6904

As always, I recommend following along by commit.

Screenshots

Desktop - expanded

Desktop - collapsed

As noted above, this is expected to be broken and will be implemented in the next PR. However, the flyout width should be correct.

Mobile - expanded

Smallest screens should have the flyout overlay to 100% width

Mobile - collapsed

QA

gh pr checkout 7027
yarn storybook
Go to http://localhost:6006/?path=/story/euicollapsiblenavbeta--kibana-example
View the demo on desktop
- Confirm that clicking the collapse button sets the flyout width correctly (should be same as the width of the toggle button)
- Confirm the toggle button toggles between left/right menu icons
- Confirm that changing the side prop in the Storybook controls panel correctly toggles the flyout position and the icon directions
View the demo on large mobile
- Confirm the flyout is now an overlay
- Confirm the icons now toggle between a menu and a cross
View the demo on small mobile
- Confirm the flyout is now full-width over the entire screen
In the Storybook controls panel, set the width prop to a custom width, e.g. 400
Resize your window appropriately to confirm that the nav's breakpoints are dynamic based on the width passed - it's considered "desktop" at 3x the width of the nav, and "smallest" at under 1.5x the width of the nav
Go to http://localhost:6006/?path=/story/euicollapsiblenavbeta--multiple-fixed-headers
Confirm the header has correctly accounted and positioned for both fixed headers

General checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest and cypress tests
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
- NOTE: This isn't quite yet ready due to the component being merged in in-progress states. I plan on making a final a11y/screenreader pass at the end / after the final PR

Below items are all N/A due to the component's beta status
~~- [ ] A changelog entry exists and is marked appropriately~~
~~- [ ] Checked for breaking changes and labeled appropriately~~
- [ ] Props have proper autodocs (using @default if default values are missing) and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Updated the Figma library counterpart~~

- separate from `EuiCollapsibleNav` itself

…ning

+ update `EuiHeader` to export its padding as var for this component to use

- on desktop, collapsing retains the flyout but in an icon only button state that toggles popovers for accordions - on small screens, the flyout reverts to an overlay and fully disappears when collapsed - on smallest possible screens, the flyout becomes 100% width (requires an `!important` to override EuiFlyout's default CSS)

- exporting/importing `_EuiFlyoutSide` directly lets us skip the potential `undefined` type of the flyout prop

cee-chen · 2023-08-02T00:43:12Z

Adding @elastic/eui-team for info, but assigning primary QA/review to @breehall. Let me know if that works for you Bree!

cee-chen · 2023-08-02T00:44:27Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+    // TODO: Context for sharing state to all children
+    <>


This will be addressed in the next PR (EuiCollapsibleNavItem will need context and cannot be passed props directly). If you want a sneak preview, you can take a look at my WIP branch.

cee-chen · 2023-08-02T00:45:16Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+  // Wait for any fixed headers to be queried before rendering (prevents position jumping)
+  const flyout = fixedHeadersCount !== false && (
+    <EuiFlyout


This seems to be new as of React 18, which seems to be a lot more aggressive about mounting/unmounting 😬 I probably need to dig deeper into React 18's changes to figure out why / see if there's a workaround.

cee-chen · 2023-08-02T00:46:01Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+
+  const classes = classNames(
+    'euiCollapsibleNav',
+    'euiCollapsibleNavBeta',


I'm planning on removing this className once this component is out of beta and replaces euiCollapsibleNav completely.

cee-chen · 2023-08-02T00:48:10Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+
+    const onWindowResize = throttle(getBreakpoints, 50);
+    window.addEventListener('resize', onWindowResize);
+    return () => window.removeEventListener('resize', onWindowResize);


I don't love adding an extra window listener just for this component, but I decided the "custom" responsive breakpoints (based on the width of the nav) felt better than just blindly using EUI's s/m sized breakpoints. I think it fits the "container query" vs "media query" paradigm better as well.

cee-chen · 2023-08-02T00:53:43Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+  /**
+   * Collapsed state
+   */
+  const [isCollapsed, setIsCollapsed] = useState(initialIsCollapsed);
+  const toggleCollapsed = useCallback(
+    () => setIsCollapsed((isCollapsed) => !isCollapsed),
+    []
+  );
+  const onClose = useCallback(() => setIsCollapsed(true), []);


Just want to note that unlike the current EuiCollapsibleNav, the new component's collapsed/expanded state is entirely self-contained (as opposed to being set/passed by consumers).

The button prop that previously exists on the old component is also now entirely handled by EuiCollapsibleNavBeta. The component is also primarily meant to be used within EuiHeader, and looks a bit odd if it's not in one (this can be overridden via custom styles if absolutely needed).

TBH, I figure if consumers really want their own custom nav/flyout, they can simply use EuiFlyout. As such, I decided to get way more opinionated about EuiCollapsibleNav going forward, since I expect primarily just Kibana to be using it. It's also a much more higher-level component compared to others, and IMO the higher level a component is the less flexible it needs to be (as opposed to lower level building blocks/atoms).

If we get a consumer request for more customization of EuiCollapsibleNav we can re-evaluate in the future, but for now I'm leaning towards expedience.

TBH, I figure if consumers really want their own custom nav/flyout, they can simply use EuiFlyout

This is fair!

kibanamachine · 2023-08-02T01:01:53Z

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

kibanamachine · 2023-08-02T01:02:57Z

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

elasticmachine · 2023-08-02T01:14:43Z

💚 Build Succeeded

Buildkite Build
Commit: ee4c1ed

breehall

Great work Cee! I just have a question or two surrounding the sizing of the menu!

breehall · 2023-08-02T18:47:43Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+    <EuiFlyout
+      {...rest} // EuiCollapsibleNav is significantly less permissive than EuiFlyout
+      id={flyoutID}
+      css={cssStyles}
+      className={classes}
+      style={style}
+      size={248} // TODO: Responsive behavior
+      side={side}
+      focusTrapProps={focusTrapProps}
+      as="nav"
+      type="push" // TODO: Responsive behavior
+      paddingSize="none"
+      pushMinBreakpoint="s"
+      onClose={() => {}} // TODO: Collapsed state
+      hideCloseButton={true}
+    >
+      {/* TODO: collapsible button */}
+      {children}
+    </EuiFlyout>


I think utilizing EuiFlyout instead of EuiCollapsibleNav is a good decision here. This component will be quite an improvement from EuiCollapsibleNav and if we decide to move away from that component in the future, we won't have to worry about "detangling" this version as much.

breehall · 2023-08-02T19:05:27Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+  /**
+   * Collapsed state
+   */
+  const [isCollapsed, setIsCollapsed] = useState(initialIsCollapsed);
+  const toggleCollapsed = useCallback(
+    () => setIsCollapsed((isCollapsed) => !isCollapsed),
+    []
+  );
+  const onClose = useCallback(() => setIsCollapsed(true), []);


TBH, I figure if consumers really want their own custom nav/flyout, they can simply use EuiFlyout

This is fair!

breehall · 2023-08-02T19:43:06Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+    if (isSmallestScreen) return '100%';
+    if (isSmallScreen) return _width;
+    if (isCollapsed) return headerHeight;


Were the three different menu sizes a requirement for this newer version of a collapsible nav? I'm thinking of current navigation and of the current offerings, and I don't think they take up the entire screen when displayed on a super small device.

Resize your window appropriately to confirm that the nav's breakpoints are dynamic based on the width passed - it's considered "desktop" at 3x the width of the nav, and "smallest" at under 1.5x the width of the nav

Can you tell me a little about why we chose to go with dynamic breakpoints?

Were the three different menu sizes a requirement for this newer version of a collapsible nav?

Yep, specific requirements from the designers. See the original issue (#6759 - Ctrl+F for "docked option") and the Figma link. The "Navigation At Smallest Viewport Sizes" pink callout in particular is very new, no other flyouts go to 100% screen width.

Can you tell me a little about why we chose to go with dynamic breakpoints?

This is somewhat answered in my above comment, but let me expound on it a bit more:

[...] I decided the "custom" responsive breakpoints (based on the width of the nav) felt better than just blindly using EUI's s/m sized breakpoints. I think it fits the "container query" vs "media query" paradigm better as well.

Let's say a consumer wants, for whatever godforsaken reason, to set their nav width to 1000px wide, and we're using the static s sized EUI breakpoint (575px) for determining when to switch to an overlay flyout. This combination is a layout nightmare for consumers on smaller screens - the navigation will push content off-screen for consumers at (e.g.) 800px wide screens. Our options at that point are:

To hope consumers don't do something that dumb or if they do, that they update/override EUI's breakpoints accordingly (wishful thinking)

To place strict guardrails around the width prop and try to do things like set a maximum width on it (can be frustrating for both parties and can inevitably lead to issues being opened when our components don't support a super custom or edge case scenario)

Or 3, and IMO the most flexible option, to base the responsive breakpoints of the component on its width, which we're already obtaining from consumers as a prop.

To be clear, sometimes 2 is the option we go with, but in this the benefits of 3 were well worth the dev effort, and, as mentioned, more closely follows the paradigm of container queries vs media queries.

Oh, I maybe missed some nuance in your question - if you're asking about where I came up with the 3x/1.5x ratios, those didn't come from the designers, I kind of eyeballed them based on what felt good/natural based on visual balance and so forth.

My reasoning for 3x is - you don't want a nav taking up more than 1/3rd the total available screen width - 1/2 definitely feels like it's massively cramping the page content. For 1.5x, anything larger made a sorta weird width jump when going from a non-full-width flyout to a full-width flyout.

Oh, I maybe missed some nuance in your question

No you got it! I wasn't sure if this was a pattern that came from designers or if we as developers were creating a new pattern, so thank you for pointing me to the info.

This definitely feels like a "training wheels" situation where we're just guiding and hoping they'll make good decisions.

Edit: Nevermind, I'm taking that thought back!

~~Have we thought about adding a minimum width as a safe guard? They shouldn't want to do that, but accidents happen~~

This is very dependent on the scenario, but in general I'm not a super huge fan of setting safeguards around props (the exception to that being accessibility requirements).

You end up with requests like this one where the consumer has a specific usage for a component that doesn't match the one we designed for, but makes total sense for their layout or their primary audience.

In this case we've designed a system that works very well for Kibana's intended nav width (248px) and will work reasonably well for most sensible nav sizes within the 100-600px ballpark (could go even higher if a consumer is using those 4000px thunder monitors; no shade). Could a consumer pass in something ridiculous like -200 and break the component? Yeah, sure, but that's not realistic and there's no sense spending time writing extra guards or tests for it - generally, trying to handle/predict reasonable use cases and ignoring unreasonable ones is going to be the sanest path forward. And sometimes we get issues opened where we determine the request to be reasonable after all and just not one we'd thought of, and that's totally okay/part of the iterative process of things.

but that's not realistic and there's no sense sitting spending time writing extra guards or tests for scenarios that it

Absolutely! And that's why I took the thought back after thinking on it a little more. Hope I didn't waste too much of your time explaining. Thank you so much for the insight!

No worries at all, I always like having these convos out in the open - there's always a chance future-us will want to go back and figure out why we did something a certain way, or even that someone totally random out there on the internet will learn something new!

breehall

This is good to go! Thank you for answering my questions in the comments!

cee-chen · 2023-08-02T21:40:12Z

Thanks a ton for the review and the engaging conversations Bree!

kibanamachine · 2023-08-02T22:06:37Z

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

cee-chen added 6 commits August 1, 2023 17:05

Set up basic EuiCollapsibleNavBeta component

39cdf2a

- separate from `EuiCollapsibleNav` itself

Account for fixed header affordance/offset in collapsible nav positio…

ab346b3

…ning

Set up basic isCollapsed state and initialIsCollapsed prop

3fbf244

Implement EuiCollapsibleNavButton

85b96cb

+ update `EuiHeader` to export its padding as var for this component to use

Misc types cleanup

ee4c1ed

- exporting/importing `_EuiFlyoutSide` directly lets us skip the potential `undefined` type of the flyout prop

cee-chen added the skip-changelog label Aug 2, 2023

cee-chen requested review from a team and breehall August 2, 2023 00:42

cee-chen commented Aug 2, 2023

View reviewed changes

breehall reviewed Aug 2, 2023

View reviewed changes

breehall self-requested a review August 2, 2023 20:40

breehall approved these changes Aug 2, 2023

View reviewed changes

cee-chen merged commit dd8d538 into elastic:main Aug 2, 2023

cee-chen deleted the collapsible-nav-beta branch August 2, 2023 21:40

cee-chen mentioned this pull request Aug 4, 2023

[EuiCollapsibleNavBeta] Final collapsed/docked icon & popover behavior #7034

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiCollapsibleNavBeta] Build out component state and responsive behavior #7027

[EuiCollapsibleNavBeta] Build out component state and responsive behavior #7027

cee-chen commented Aug 2, 2023 •

edited

Loading

cee-chen commented Aug 2, 2023

cee-chen Aug 2, 2023

cee-chen Aug 2, 2023

cee-chen Aug 2, 2023

cee-chen Aug 2, 2023

cee-chen Aug 2, 2023 •

edited

Loading

breehall Aug 2, 2023

kibanamachine commented Aug 2, 2023

kibanamachine commented Aug 2, 2023

elasticmachine commented Aug 2, 2023

breehall left a comment

breehall Aug 2, 2023

breehall Aug 2, 2023

breehall Aug 2, 2023

breehall Aug 2, 2023

cee-chen Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

breehall Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

breehall Aug 2, 2023

cee-chen Aug 2, 2023 •

edited

Loading

breehall left a comment

cee-chen commented Aug 2, 2023

kibanamachine commented Aug 2, 2023

[EuiCollapsibleNavBeta] Build out component state and responsive behavior #7027

[EuiCollapsibleNavBeta] Build out component state and responsive behavior #7027

Conversation

cee-chen commented Aug 2, 2023 • edited Loading

Summary

Screenshots

Desktop - expanded

Desktop - collapsed

Mobile - expanded

Mobile - collapsed

QA

General checklist

cee-chen commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 2, 2023

kibanamachine commented Aug 2, 2023

elasticmachine commented Aug 2, 2023

💚 Build Succeeded

breehall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

breehall Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

breehall left a comment

Choose a reason for hiding this comment

cee-chen commented Aug 2, 2023

kibanamachine commented Aug 2, 2023

cee-chen commented Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

breehall Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading