[EuiSideNav] Fixes and enhancements for the Elastic Solution Nav by cchaos · Pull Request #4827 · elastic/eui

cchaos · 2021-05-26T21:14:48Z

The following screenshots are progressive. Meaning they were taken as the updates were completed so they don't all encompass the final result.

1. Fixes

A. Fixed [EuiPageBody] shadow overlapping emphasized side nav

By adding a z-index: 1 to the page body. I'll have to monitor any downstream effects of this.

B. Better color/transparency for emphasized and proper contrast

Updated background color to be transparent for use in the emphasis mixin (see below) and updated all the calculations including disabled text. There's not much of a visual change.

C. Reduced display of arrow icon to only if the item is not linked but has children

And the component maintains the open status within itself. This drastically reduces noise when all or most of the items have children. It is also the functionality that will help solution teams like Observability create items that aren't actually navigable themselves but have children that are.

D. Fixed up mobile button styles to be more prominent

The button which may be the primary navigation's trigger was rendering quite small. Now it will stretch as tall as its content and the font size is larger.

2. Amsterdam style

A. Reduced the spacing and sizing of root element for Amsterdam only

3. Additions

A. Added `heading` and `hideHeading` props for better a11y

And more aria props to mobile button. Talking with @myasonik, the best approach for labelling sections is to have a nested heading element. Since it's also a pretty safe bet that the rendered <nav> will be at the top level, the <h2> heading level is hard-coded.

This h2 is also then used as the mobile title if a specific one is not supplied, making it easier to create good mobile button labels.

B. Added a "hidden" Sass mixin for re-creating the branded nav embellishment.

It's called euiSideNavEmbellish() and looks like this:

4. Other changes

A. Added `mobile` glyph to EuiIcon.

The final before/after.

I think the removal of the repetitious arrows is a big improvement on scannability overall.

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
Added documentation
Checked Code Sandbox works for the any docs examples
Added or updated jest tests
~~[ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

…in open status within

And more aria props to mobile button

kibanamachine · 2021-05-26T21:38:48Z

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

kibanamachine · 2021-05-26T22:05:17Z

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

myasonik · 2021-05-26T22:57:10Z

Just some early feedback: Code review looks good

Want to check out the docs preview before approving

chandlerprall

Couple of small nits, but otherwise this looks good.

[Future] I agree with removing the arrow for interactive parent items, but I wonder if we should have the ability to expand parents without triggering their action/navigation

src/components/side_nav/side_nav.tsx

src/components/side_nav/side_nav_item.tsx

myasonik

A few small things but otherwise looks good!

myasonik · 2021-06-02T14:31:52Z

src/components/side_nav/side_nav.tsx

+          iconSide="right"
+          aria-controls={sideNavContentId}
+          aria-expanded={isOpenOnMobile}
+          aria-haspopup="true">


Suggested change

aria-haspopup="true">

aria-haspopup has 5 values plus a boolean: menu, grid, dialog, listbox, and tree. true is the same as menu. false is the same as not including it but can indicate that a popup might exist in other situations.

Any of those 5 values then expect that role to be the thing that opens up in some sort of dialog. So a simple open/close pattern doesn't require it.

🆒 Thanks for the info. I totally just copy/pasted from a popover menu. 😺

myasonik · 2021-06-02T14:40:49Z

src/components/side_nav/side_nav.tsx

+          aria-expanded={isOpenOnMobile}
+          aria-haspopup="true">
+          {/* Inline h2 ensures truncation */}
+          {mobileTitle || <h2 className="eui-displayInline">{heading}</h2>}


Any semantic content inside of a button gets collapsed into a plain string so we need to move this h2 out from inside. A button inside of an h2 is fine though 👍

(You can see for yourself by opening up this button/heading demo in Safari and opening up VoiceOver. Opening up the rotor (ctrl+opt+u) and arrowing left/right until you find the headings list.)

myasonik · 2021-06-02T14:44:33Z

src/components/side_nav/side_nav.tsx

+          aria-expanded={isOpenOnMobile}
+          aria-haspopup="true">
+          {/* Inline h2 ensures truncation */}
+          {mobileTitle || <h2 className="eui-displayInline">{heading}</h2>}


Suggested change

{mobileTitle || <h2 className="eui-displayInline">{heading}</h2>}

{mobileTitle || <h2 id={uniqueId} className="eui-displayInline">{heading}</h2>}

Doing this with the mobileTitle is tricky but we can at least do it with the h2 that we control: setting up a uniqueId and adding an aria-labelledby={uniqueId} to the wrapping nav element will ensure that the nav is always properly named regardless of browser/assistive tech.

kibanamachine · 2021-06-02T18:53:00Z

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

This also meant moving the responsive behavior to JS with Show/HideFor components with the ability to adjust via `mobileBreakpoints` and further heading adjustments with `headingElement`

# Conflicts: # CHANGELOG.md

sass-lint update: Excluding specific mixins from being forced to the top

cchaos · 2021-06-02T22:15:53Z

Update

In order to support the ariaLabelledBy as suggested by @myasonik, I had to move all the responsive hide/show logic into the JSX. This made a lot of logic more complicated because there may/may not be a header on desktop, but there always is in mobile, but now I've also made the mobileBreakpoints adjustable as well as the headingElement.

myasonik

All good! One kind of meta question on props though...

We have a few heading related props: heading, headingElement, hideHeading, and headingProps.

I can understand why/how each exists and developed but I wonder if we should include hideHeading and headingElement inside of headingProps?

On one hand, headingProps is right now only a subset of the existing EuiTitleProps but, on the other hand, it thematically makes some sense to include these new heading props under and object called headingProps.

I'm not sure where the balance of a clean API for this component vs a consistent API across components comes down for this one...

kibanamachine · 2021-06-02T22:39:01Z

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

elizabetdev

LGTM! 🎉

Tested in Chrome, Safari, Edge, and Firefox.

The only thing that I noticed is that the new callouts are smaller than the one on the bottom of the page:

New one (I thought I had the zoom out):

And in other pages like https://eui.elastic.co/pr_4663/#/navigation/collapsible-nav we're using the default size. So should we decide what is the size to be used across all pages?

cchaos · 2021-06-03T15:18:44Z

Thanks @miukimiu , I agree we need better consistency. I'll adjust the new one I created to be the medium size and we should probably continue to use that when directly in the docs pages and maybe restrict small to uses within components.

cchaos · 2021-06-03T15:19:33Z

@myasonik That's a good idea, since these are new props I'll adjust now and hope they're discoverable enough. I'll probably need to add more explanation in the docs to help with that.

chandlerprall

Docs & code changes LGTM! 🦑

cchaos · 2021-06-07T20:36:24Z

Unfortunately the latest change borked the display when the component re-renders because of search. You can the issue if you search for a page in the docs.

Duplicates a lot of rendered CSS, but can be fixed later with CSS-in-JS

# Conflicts: # CHANGELOG.md

kibanamachine · 2021-06-07T21:54:35Z

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

kibanamachine · 2021-06-10T20:18:28Z

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

cchaos added 9 commits May 25, 2021 13:55

Fixed [EuiPageBody] shadow overlapping emphasized side nav

f885fcc

Better color/transparency for emphasized and proper contrast

9679d81

[Amsterdam] Updating spacing and sizing of root element

6a838e7

Only show arrow icon if item is no linked but has children and mainta…

bfead1a

…in open status within

Added heading and hideHeading props for better a11y

7c8b9e6

And more aria props to mobile button

Added mobile glyph to EuiIcon

6d73167

Docs

7246361

Added embellishement mixin

1a027c3

Merge remote-tracking branch 'upstream/master' into fixes/side_nav

f1f65f4

cl

7746bf2

cchaos requested review from elizabetdev and myasonik May 26, 2021 21:41

chandlerprall self-requested a review June 1, 2021 17:15

chandlerprall approved these changes Jun 1, 2021

View reviewed changes

src/components/side_nav/side_nav.tsx Outdated Show resolved Hide resolved

src/components/side_nav/side_nav_item.tsx Outdated Show resolved Hide resolved

myasonik approved these changes Jun 2, 2021

View reviewed changes

Easy fixes from review

985509c

cchaos added 6 commits June 2, 2021 14:59

Moving h2 to wrap button and adding headingProps to adjust EuiTitle

5a44fae

WIP - HeadginElement help

c84874f

Added ariaLabelledBy for better a11y

4bef1fb

This also meant moving the responsive behavior to JS with Show/HideFor components with the ability to adjust via `mobileBreakpoints` and further heading adjustments with `headingElement`

Unrelated docs change: Fix empty state icon color

dfc9e25

Merge remote-tracking branch 'upstream/master' into fixes/side_nav

49bbff5

# Conflicts: # CHANGELOG.md

Fixing content styles in mobile adjacent to JS

1c3909b

sass-lint update: Excluding specific mixins from being forced to the top

cchaos requested review from chandlerprall and myasonik June 2, 2021 22:16

myasonik approved these changes Jun 2, 2021

View reviewed changes

elizabetdev approved these changes Jun 3, 2021

View reviewed changes

chandlerprall approved these changes Jun 7, 2021

View reviewed changes

cchaos and others added 4 commits June 7, 2021 17:20

Merged heading props into one headingProps

61ec735

Fixed double padding when isOpenMobile

8bf3057

Duplicates a lot of rendered CSS, but can be fixed later with CSS-in-JS

Merge remote-tracking branch 'upstream/master' into fixes/side_nav

18d812e

# Conflicts: # CHANGELOG.md

Update CHANGELOG.md

0769955

cchaos merged commit 5ec3d51 into elastic:master Jun 8, 2021

cchaos deleted the fixes/side_nav branch June 8, 2021 13:38

chandlerprall mentioned this pull request Jun 10, 2021

Upgrade EUI to v34.3.0 elastic/kibana#101334

Merged

	{mobileTitle \|\| <h2 className="eui-displayInline">{heading}</h2>}
	{mobileTitle \|\| <h2 id={uniqueId} className="eui-displayInline">{heading}</h2>}

Conversation

cchaos commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Fixes

A. Fixed [EuiPageBody] shadow overlapping emphasized side nav

B. Better color/transparency for emphasized and proper contrast

C. Reduced display of arrow icon to only if the item is not linked but has children

D. Fixed up mobile button styles to be more prominent

2. Amsterdam style

A. Reduced the spacing and sizing of root element for Amsterdam only

3. Additions

A. Added heading and hideHeading props for better a11y

B. Added a "hidden" Sass mixin for re-creating the branded nav embellishment.

4. Other changes

A. Added mobile glyph to EuiIcon.

Checklist

Uh oh!

kibanamachine commented May 26, 2021

Uh oh!

kibanamachine commented May 26, 2021

Uh oh!

myasonik commented May 26, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

myasonik Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

myasonik Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

myasonik Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 2, 2021

Uh oh!

cchaos commented Jun 2, 2021

Update

Uh oh!

myasonik left a comment • 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!

kibanamachine commented Jun 2, 2021

Uh oh!

elizabetdev left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos commented Jun 3, 2021

Uh oh!

cchaos commented Jun 3, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos commented Jun 7, 2021

Uh oh!

kibanamachine commented Jun 7, 2021

Uh oh!

kibanamachine commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cchaos commented May 26, 2021 •

edited

Loading

A. Added `heading` and `hideHeading` props for better a11y

A. Added `mobile` glyph to EuiIcon.

myasonik left a comment •

edited

Loading