[Beta] Add `EuiCollapsibleNavItem` component by cee-chen · Pull Request #6904 · elastic/eui

cee-chen · 2023-07-05T17:08:13Z

Summary

This PR sets up a new EuiCollapsibleNavItem component that follows the new nav redesign and UX outlined in #6759.

This PR, as noted by the [Beta] label in the title, is not a complete feature in and of itself. However, I'm choosing to ship it to main for the following reasonns reasons:

Creating new components/folders does not interfere or cause interruptions to consumers using the old components, and is relatively risk free
Releasing it (unannounced/undocumented) allows @tsullivan to incrementally use redesign progress in his Kibana feature work and battle-test our component using production data, leading to issues (hopefully) getting caught faster

QA

gh pr checkout 6904
yarn storybook (may need to yarn beforehand if you haven't run EUI locally in a while)
Go to http://localhost:6006/?path=/story/euicollapsiblenavbeta--kibana-example
Go to http://localhost:6006/?path=/story/euicollapsiblenavitem--edge-case-testing
Click around / QA the various links and displays and confirm they look and behave coherently (in particular, sub-sub-item nesting)

General checklist

Checked in both light and dark modes
- [ ] Checked in mobile Responsive/mobile work isn't yet done, will be part of the next upcoming work where the EuiCollapsibleNavBeta component itself gets fleshed out more
Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest ~~and cypress tests~~
Checked for accessibility including keyboard-only and screenreader modes

Changelog & docs: Skipping this for now due to beta status & until we have something we're ready for all public consumers to use. Once all components are put together, we can deprecate the old component and update docs.
~~- [ ] A changelog entry exists and is marked appropriately~~
- [ ] Props have proper autodocs (using @default if default values are missing) and playground toggles
- [ ] Added documentation

Figma: See linked issue with Figma URLs - will likely need to update EUI's Figma library after this work is all over
~~- [ ] Updated the Figma library counterpart~~

kibanamachine · 2023-07-05T17:34:05Z

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

kibanamachine · 2023-07-05T19:29:34Z

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

kibanamachine · 2023-07-06T03:11:15Z

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

kibanamachine · 2023-07-19T01:32:02Z

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

- Because of the potentially recursive nature of this component, splitting it up into multiple sub-components that differentiate styling by an `isSubItem` prop is the easiest way to go

- Full component work will come in later PR

cee-chen · 2023-07-19T01:52:23Z

@ryankeairns - adding you as a design reviewer in case you see any last minute UI/UX issues with the component stories. As a quick heads up, mobile/responsive behavior isn't in this PR yet (going work on that / the docked popover behavior in the next PR).

@breehall - adding you as an engineering reviewer. If you see any visual UI/UX issues definitely feel free to shout on that end as well of course, but in particular I'm looking for any thoughts you have on architecture and developer experience, in particular the props API from a consumers POV. For example, does the items data structure in the stories feel elegant and consistent with other EUI components, or hard to use/understand?

Also, I know there's a lot of lines in this PR - but I think they're mostly stories/snapshots/tests. Hopefully the actual source code/components themselves aren't too onerous to grok, although if they are, I definitely want that feedback as well!

+ add component comment for future-proofing

kibanamachine · 2023-07-19T17:16:17Z

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

kibanamachine · 2023-07-19T17:19:28Z

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

breehall

This looks really good so far Cee! I just had one UI issue I found, but I really love the API for the items array. I think it's easy to compose and maintain.

For titles that have children and also a link, I really like that the accordion doesn't toggle when you select the link. Very small thing, but it can be really annoying so I'm glad to see this!

The styles, tests, and snapshots look great so far as well. We had a chance to talk about the circular dependency decision today and I'm glad we did because that context helped me understand this code more. Maybe this could be an opportunity (in the future) to document this component with its subcomponents to for easy understanding of how they work together.

breehall · 2023-07-19T18:15:21Z

src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_link.tsx

+      href={href}
+      rel={rel}
+      className={classes}
+      {...({ ...rest, ...linkProps } as any)} // EuiLink ExclusiveUnion shenanigans


That good 'ol exclusive union!

hide_the_pain_harold.jpg

breehall · 2023-07-19T18:19:26Z

src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_item.tsx

+  /**
+   * Optional icon to render to the left of title content
+   */
+  iconType?: IconType;


What do you think about adding an iconProps prop to allow consumers to change the size, color, etc. of the icon? I thought of this because we're allowing customizations the link and accordion props.

Nice! I'm good with that! Also I think I mentioned this on Slack recently, I'll probably be renaming iconType to just icon.

breehall · 2023-07-19T18:33:55Z

src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_accordion.tsx

+        // This is an intentional circular dependency between the accordion & parent item display.
+        // EuiSideNavItem is purposely recursive to support any amount of nested sub items,
+        // and split up into separate files/components for better dev readability


breehall · 2023-07-19T18:43:42Z

src/components/collapsible_nav_beta/collapsible_nav_beta.stories.tsx

+          items={[
+            { title: 'Lorem ipsum', iconType: 'visMapRegion', href: '#' },
+            { title: 'Consectetur cursus', iconType: 'visPie', href: '#' },
+            { title: 'Ultricies tellus', iconType: 'visMetric', href: '#' },
+          ]}
+        />


I really like items API. It's very easy to compose and read! This is a big improvement from the composing EuiCollapsibleNav.

Yay!! Thank you for that feedback, that's great to hear! ❤️‍🔥

breehall · 2023-07-19T18:49:53Z

src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_item.stories.tsx

+            { ...args, title: 'Link with icon', href: '#', iconType: 'alert' },
+          ]}
+        />
+        <EuiCollapsibleNavItem


In this section, I'm noticing a very slight issue with tabbing. When tabbing through the menu, I'm noticing that the focus is shifted to the accordions inside of this section even when it's collapsed.

What I did: Tab all the way through the menu and when you get to Accordion with Nest Accordions section (collapsed), notice that it takes two tabs to focus on the next menu item.

Since it's collapsed as I'm tabbing, I would expect the focus to skip the child contents and jump to the next focusable item.

I'm not sure if this is something you want to address in this PR, or the next one coming up!

Screen.Recording.2023-07-19.at.2.04.42.PM.mov

Looking into this now! This may be an issue with EuiAccordion itself - if so, I probably won't fix this in this PR but instead open up a follow-up issue to resolve later.

Yep, it's reproducible on main:

https://codesandbox.io/s/withered-moon-hc3ft6?file=/demo.js

Basically the issue appears to be nested EuiAccordions, and only when initialIsOpen is set on the child/nested element. We essentially need check to see if the the accordion is in a visible/tabbable context before allowing it to set itself to tabIndex={0}.

Super awesome catch Bree! I'll look into a fix outside this PR like I mentioned.

Awesome! Sounds like a plan!

- per recent team discussion on prop naming

breehall

Hey Cee, thanks for answering my questions / comments. It looks like everything has been addressed and plans have been made!

kibanamachine · 2023-07-20T18:34:38Z

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

ryankeairns

I ran the storybook and everything looked sharp.
I did not scrutinize the code closely, but a quick review looked good from the parts I am skilled to review ;)

I'm personally comfortable with it, esp given its Beta status.

cee-chen · 2023-07-21T14:22:28Z

Thanks y'all! 🚢

cee-chen force-pushed the collapsible-nav-beta branch from f608b29 to 6537772 Compare July 5, 2023 19:06

cee-chen force-pushed the collapsible-nav-beta branch from 6537772 to 37d0e91 Compare July 6, 2023 02:47

cee-chen mentioned this pull request Jul 6, 2023

Updates to navigation components in support of Kibana nav redesign #6759

Closed

cee-chen force-pushed the collapsible-nav-beta branch from 37d0e91 to 053e496 Compare July 19, 2023 01:08

cee-chen changed the title ~~Collapsible nav beta~~ [Beta] Add EuiCollapsibleNavItem component Jul 19, 2023

cee-chen added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Jul 19, 2023

cee-chen added 4 commits July 18, 2023 18:42

Set up EuiCollapsibleNavItem component + subcomponents

da8f3a1

- Because of the potentially recursive nature of this component, splitting it up into multiple sub-components that differentiate styling by an `isSubItem` prop is the easiest way to go

Set up extremely light EuiCollapsibleNav beta styles

e13b24e

- Full component work will come in later PR

Add detailed Stories for QA testing

fc7b7b4

Exports

15d74be

cee-chen force-pushed the collapsible-nav-beta branch from 053e496 to 15d74be Compare July 19, 2023 01:42

cee-chen marked this pull request as ready for review July 19, 2023 01:42

cee-chen requested review from breehall and ryankeairns July 19, 2023 01:43

cee-chen force-pushed the collapsible-nav-beta branch from 1d941ac to e48f0f9 Compare July 19, 2023 16:51

[docs webpack] ignore circular dependency error

7272344

+ add component comment for future-proofing

cee-chen force-pushed the collapsible-nav-beta branch from e48f0f9 to 7272344 Compare July 19, 2023 16:54

breehall reviewed Jul 19, 2023

View reviewed changes

cee-chen added 2 commits July 20, 2023 11:00

Change iconType to icon

a649a81

- per recent team discussion on prop naming

[PR feedback] Add optional iconProps

84d97f7

breehall approved these changes Jul 20, 2023

View reviewed changes

cee-chen mentioned this pull request Jul 20, 2023

[EuiAccordion] Children are still present in keyboard tab order when accordions are closed #6980

Closed

ryankeairns approved these changes Jul 20, 2023

View reviewed changes

cee-chen merged commit 0ccd0c7 into elastic:main Jul 21, 2023

cee-chen deleted the collapsible-nav-beta branch July 21, 2023 14:22

This was referenced Aug 2, 2023

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

Merged

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

Merged

weronikaolejniczak mentioned this pull request Mar 18, 2025

[Beta] EuiCollapsibleNavBeta functionality #8458

Closed

5 tasks

Conversation

cee-chen commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

kibanamachine commented Jul 5, 2023

Uh oh!

kibanamachine commented Jul 5, 2023

Uh oh!

kibanamachine commented Jul 6, 2023

Uh oh!

kibanamachine commented Jul 19, 2023

Uh oh!

cee-chen commented Jul 19, 2023

Uh oh!

kibanamachine commented Jul 19, 2023

Uh oh!

kibanamachine commented Jul 19, 2023

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 20, 2023

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Jul 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Jul 5, 2023 •

edited

Loading

cee-chen Jul 20, 2023 •

edited

Loading