[EuiCollapsibleNavBeta] Final collapsed/docked icon & popover behavior by cee-chen · Pull Request #7034 · elastic/eui

cee-chen · 2023-08-04T05:52:53Z

Summary

closes Updates to navigation components in support of Kibana nav redesign #6759 (although I'm still planning on one more PR after this which will contain an a11y pass)
✅ This PR is part 2 of 2 of completing EuiCollapsibleNavBeta's responsive/docked behavior.
- I opted to split out this work into separate PRs, as each PR was already fairly high in LOCs/review time
Previous PRs: [Beta] Add EuiCollapsibleNavItem component #6904, [EuiCollapsibleNavBeta] Build out component state and responsive behavior #7027

This PR implements the final collapsed/docked behavior on desktop/wider screens:

Each top-level navigation item should always have a tooltip
Navigation items with sub-items (would render an accordion normally) will instead render a popover
Navigation items with no sub-items will render a button/link as they would normally. Unlike non-collapsed items, a static <span> will never be rendered, as tooltips require focusable anchors for accessibility.

As always, there's a lot going on here and some commits specifically dedicated to cleaning up some things I missed in the previous PR, so follow along by commit!

QA

gh pr checkout 7034
yarn storybook

EuiCollapsedNavItem testing

Go to http://localhost:6006/?path=/story/euicollapsednavitem--playground
Play with the icon control and confirm it can be customized
Clear the icon control and confirm it falls back to a 'boxes' icon
Hover over the popover and confirm it renders a tooltip with the item title
Open the popover confirm the title is also displayed as a popover title, and can be a link
Clear the href control and confirm the popover title is no longer a link / is a static span
Edit the title control to a really long string and confirm the popover title truncates
Clear the items object of all keys and confirm the popover disappears, and the icon (if empty) now falls back to a link icon
Toggle the isSelected control and confirm a background color is applied

EuiCollapsibleNavBeta testing

Go to http://localhost:6006/?path=/story/euicollapsiblenavbeta--kibana-example
Ensure you're on desktop mode, then click the collapse button
Confirm all top-level collapse to their icons and render the expected tooltips/popovers
Confirm that on popover open, the tooltip disappears
Regression testing
- Open the Security item and confirm the nested accordion items work as expected
- On mobile screens, confirm the collapsible nav still retains its flyout / full-width behavior

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~~ While this is an "okay" experience currently, it could certainly be better. I plan on doing a separate a11y polish pass PR after this, and looping in Trevor for his expertise.

Skipping due to beta component 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~~

- this isn't required by this PR, I just noticed it while working and wanted to clean up the className output of various sub-components to match their actual usage/component names

- `linkProps` should only be spread to actual EuiLink components, and not to the `span` - in reality, I'm not super sure how much of a difference this will make to consumers, but the upcoming collapsed nav item work will also behave this way and it's better to be consistent

- this may seem silly right now for just `EuiCollapsibleNavButton`, but it'll be needed by EuiCollapsibleNavBeta's children and grandchildren which we can't simply pass props directly to

- will be used by all top level items in collapsed/docked desktop state

- Similar to how `EuiCollapsibleNavAccordion` dogfoods `EuiCollapsibleNavLink`, this component dogfods `EuiCollapsedNavButton`

- which is a light wrapper similar to `EuiCollapsibleNavItemDisplay` that determines whether to render a nav component with or without items

…psibleNavItem`

- from both the nav toggle button and all docked button icons + fix missing `className`/`css` merging & tests on `EuiCollapsibleNavButton` - in theory EUI's the only ones using the component, but might as well make it flexible

- for mouse users, the cursor is still hovered over the button icon even when the popover is open, so the tooltip overlays on the popover title when it shouldn't - for keyboard users, this isn't an issue

kibanamachine · 2023-08-04T06:15:41Z

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

- scrollbars that take up width (e.g. windows, mac setting) will cut too much into icon visibility otherwise. This CSS visually hides the scrollbars for all supported browsers while still allowing the containers to remain scrollable

cee-chen · 2023-08-04T06:25:41Z

src/components/collapsible_nav_beta/collapsible_nav_button/collapsible_nav_button.styles.ts

+    euiCollapsibleNavButton: css`
+      &.euiButtonIcon:hover {
+        transform: none;
+      }
+    `,


This commit (965b476) was a request from Marcialis. I missed implementing it as part of the previous PR so figured I'd grab it in here.

The &.euiButtonIcon specificity is unfortunately necessary to override EuiButtonicon's existing hover CSS.

cee-chen · 2023-08-04T06:27:52Z

...omponents/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.styles.ts

+    // If the item has a popover and the popover is open, we don't want the
+    // tooltip to appear if so - the popover already renders the item title
+    hidden: css`
+      display: none;
+    `,


Without this extra logic/CSS, you get this silliness:

You can still overlap the tooltips of other nav items onto open popovers, but at least that doesn't repeat information.

Great catch!

cee-chen · 2023-08-04T06:31:11Z

src/components/collapsible_nav_beta/collapsible_nav_beta.styles.ts

+      /* This extra padding is needed for EuiPopovers to have enough
+         space to render with the right anchorPosition */
+      ${logicalCSS('padding-bottom', euiTheme.size.xs)}


Otherwise we get this:

When we want this:

cee-chen · 2023-08-04T06:33:50Z

src/components/collapsible_nav_beta/collapsible_nav_beta.styles.ts

+    isDesktopCollapsed: css`
+      /* Hide the scrollbar for docked mode (while still keeping the nav scrollable) 
+         Otherwise if scrollbars are visible, button icon visibility suffers */
+      &,
+      .euiFlyoutBody__overflow {
+        scrollbar-width: none; /* Firefox */
+
+        &::-webkit-scrollbar {
+          display: none; /* Chrome, Edge, & Safari */
+        }
+      }
+    `,


Without this conditional CSS, you get this horrible shenanigans:

Which, I guarantee, will unfortunately only look worse on Windows 😬

I'm opting to only set this on for the docked/icon mode, as the normal nav items display will adjust to accommodate a scrollbar width.

Quick note - I just pushed up a naming refactor (a582ddc), but that should be my last set of changes / this PR is all ready to review after that :)

cee-chen · 2023-08-04T06:38:17Z

...components/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.test.tsx

Just want to note - I know this test (and dev tools) triggers this warning repeatedly:

I'm opting to skip it for now and tackle it in a separate accessibility PR/pass. The main reason it's actually not a major a11y issue is because of the wrapping EuiToolTip, which sets an aria-describedby on the button icon.

So the title will still get read out, albeit less quickly than an aria-label would. But if I try to set an aria-label now, what happens is the title then gets repeated twice due to the tooltip describedby, which TBH feels worse. So I'd rather try to fix the issue more completely, hence the separate follow-up work.

kibanamachine · 2023-08-04T06:43:25Z

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

kibanamachine · 2023-08-04T06:43:32Z

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

- I totally missed this in the Figma mocks 🥲

- instead of using `small`/`mobile`/`desktop`, use `push` vs `overlay` naming instead, which better describes what state the flyout is in - this matches the paradigm we want of container vs media queries - we should be describing how the *flyout* is behaving, not what size we think the user's screen is

kibanamachine · 2023-08-04T18:50:40Z

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

kibanamachine · 2023-08-04T18:55:34Z

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

breehall

This looks good overall. I spent a good amount of time with the collapse functionality and verifying across different screen sizes. The ability to navigate when the menu is collapsed (on desktop) is a nice touch!

[Consideration] I also spent some time with keyboard navigation. I know another PR for accessibility is coming down the line, but wanted to throw in this note. When the menu is collapsed and you're using the keyboard to navigate to the first item in the menu, it takes three tab pressed to land there. I don't think this is the same as the tabbing issue we identified in the last PR because there are no children between the collapse button and the home item.

[Change request] It looks like the arrow states for the accordions are backwards. When the parent is collapsed, I believe the arrow should point down. When the parent is open, the parent should point up. I double checked with this with the Figma spec. I'm not sure how I missed that before, sorry about that!

breehall · 2023-08-04T19:53:11Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+  const [isOverlay, setIsOverlay] = useState(false);
+  const [isOverlayFullWidth, setIsOverlayFullWidth] = useState(false);


I like this change! It's very intuitive.

Music to my ears!! 😁

breehall · 2023-08-04T20:40:41Z

src/components/collapsible_nav_beta/collapsible_nav_beta.styles.ts

+      .euiFlyoutBody {
+        ${logicalCSS('min-height', '50%')}
+      }
+


Good call on this. I toyed around by opening all of the toggles between the flyout body and footer. Hopefully no one would do this, but this is nice just in case.

Haha, I remembered that one issue/bug report we got previously and tried to get ahead of it! :)

breehall · 2023-08-04T20:44:51Z

...omponents/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.styles.ts

+    // If the item has a popover and the popover is open, we don't want the
+    // tooltip to appear if so - the popover already renders the item title
+    hidden: css`
+      display: none;
+    `,


Great catch!

cee-chen · 2023-08-04T23:37:03Z

[Change request] It looks like the arrow states for the accordions are backwards.

🤦 You're totally right, thank you so much for catching that! 2a1ec67

cee-chen · 2023-08-04T23:45:14Z

[Consideration] I also spent some time with keyboard navigation. I know another PR for accessibility is coming down the line, but wanted to throw in this note. When the menu is collapsed and you're using the keyboard to navigate to the first item in the menu, it takes three tab pressed to land there. I don't think this is the same as the tabbing issue we identified in the last PR because there are no children between the collapse button and the home item.

Yep, great catch on this, I noticed the same thing. This behavior is a factor of several issues: Push flyouts in general have odd accessibility compared to non-push flyouts (see #6576), and also the euiFlyoutBody__overflow has a non-configurable tabIndex={0}. So the extra tab stops are being caused the flyout itself (which can be resolved by auto-focusing the flyout on open state, which all push flyouts should do), and the scrolling region needs to be able to intelligently detect whether or not it needs a tabIndex={0} instead of just setting one by default.

All of the above are mediumly complex issues to resolve that I don't want to hold up Kibana's work for, since we can continue to work on them in the background and a11y fixes should Just Work later. 🤞

kibanamachine · 2023-08-05T00:04:59Z

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

kibanamachine · 2023-08-05T00:06:36Z

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

elasticmachine · 2023-08-05T00:15:59Z

💚 Build Succeeded

Buildkite Build
Commit: 2a1ec67

History

💚 Build #206 succeeded a582ddc
💚 Build #196 succeeded f9838a3

breehall

Thank you for addressing my questions and flipping the accordion arrow!

cee-chen added 10 commits August 3, 2023 21:37

[misc] Clean up EuiCollapsibleNavItem className rendering

5dff814

- this isn't required by this PR, I just noticed it while working and wanted to clean up the className output of various sub-components to match their actual usage/component names

Set up EuiCollapsibleNavContext

5b7869b

- this may seem silly right now for just `EuiCollapsibleNavButton`, but it'll be needed by EuiCollapsibleNavBeta's children and grandchildren which we can't simply pass props directly to

Set up EuiCollapsedNavButton component

b19eddc

- will be used by all top level items in collapsed/docked desktop state

Set up EuiCollapsedNavPopover component

5e22be8

- Similar to how `EuiCollapsibleNavAccordion` dogfoods `EuiCollapsibleNavLink`, this component dogfods `EuiCollapsedNavButton`

Set up EuiCollapsedNavItem component

1cff317

- which is a light wrapper similar to `EuiCollapsibleNavItemDisplay` that determines whether to render a nav component with or without items

Final logic for switching between EuiCollapsedNavItem and `EuiColla…

0c3cb8d

…psibleNavItem`

[Designer feedback] Remove button hover effect

965b476

- from both the nav toggle button and all docked button icons + fix missing `className`/`css` merging & tests on `EuiCollapsibleNavButton` - in theory EUI's the only ones using the component, but might as well make it flexible

[polish] Fix tooltips overlapping popovers

0fce843

- for mouse users, the cursor is still hovered over the button icon even when the popover is open, so the tooltip overlays on the popover title when it shouldn't - for keyboard users, this isn't an issue

[polish] Fix bottom-most button not having a right-anchored popover

17b8c62

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

cee-chen force-pushed the collapsible-nav-beta branch from 113b497 to f9838a3 Compare August 4, 2023 06:19

cee-chen requested a review from breehall August 4, 2023 06:38

cee-chen commented Aug 4, 2023

View reviewed changes

cee-chen mentioned this pull request Aug 4, 2023

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

Closed

cee-chen changed the title ~~[EuiCollapsibleNavBeta] Final collapsed/dock icon/popover behavior~~ [EuiCollapsibleNavBeta] Final collapsed/docked icon & popover behavior Aug 4, 2023

cee-chen added 2 commits August 4, 2023 10:56

[Designer feedback] Add slight shadow to nav

d6c7f7e

- I totally missed this in the Figma mocks 🥲

cee-chen force-pushed the collapsible-nav-beta branch from 81f63ba to a582ddc Compare August 4, 2023 18:27

breehall reviewed Aug 4, 2023

View reviewed changes

[PR design feedback] Accordion arrows are rotated the wrong way 🤦

2a1ec67

breehall approved these changes Aug 7, 2023

View reviewed changes

cee-chen merged commit 8dcf472 into elastic:main Aug 7, 2023

cee-chen deleted the collapsible-nav-beta branch August 7, 2023 14:50

cee-chen mentioned this pull request Aug 7, 2023

[EuiCollapsibleNavBeta] Add storybook QA test for ensuring flyouts sit above the nav #7051

Merged

1 task

tsullivan mentioned this pull request Aug 23, 2023

Incorporate EuiCollapsibleNavItem into side nav elastic/kibana#162507

Closed

		const [isOverlay, setIsOverlay] = useState(false);
		const [isOverlayFullWidth, setIsOverlayFullWidth] = useState(false);

Conversation

cee-chen commented Aug 4, 2023 • edited by breehall Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

kibanamachine commented Aug 4, 2023

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!

kibanamachine commented Aug 4, 2023

Uh oh!

kibanamachine commented Aug 4, 2023

Uh oh!

kibanamachine commented Aug 4, 2023

Uh oh!

kibanamachine commented Aug 4, 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!

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

Uh oh!

Uh oh!

cee-chen commented Aug 4, 2023

Uh oh!

kibanamachine commented Aug 5, 2023

Uh oh!

kibanamachine commented Aug 5, 2023

Uh oh!

elasticmachine commented Aug 5, 2023

💚 Build Succeeded

History

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Aug 4, 2023 •

edited by breehall

Loading

cee-chen commented Aug 4, 2023 •

edited

Loading