[New Nav Feature] Final docs examples and patterns by cchaos · Pull Request #3117 · elastic/eui

cchaos · 2020-03-18T14:56:46Z

This PR is primarily docs

1. Example showing different types of children that can be passed to EuiCollapsibleNavGroup

Including EuiPinnableListGroup, simple EuiListGroup, custom content to create a callout style.

2. Full screen example with header and saved pinning/open/closed states

This saves the states in local storage, but encourages consumers to use the correct data store for their application.

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in IE11 and Firefox
[ ] Props have proper autodocs
Added documentation 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~~ Will be added in feature PR

cchaos · 2020-03-18T15:06:12Z

The main reason this is a Draft is because I tried creating a wrapping component guides that does the full screen overlay. The main reason I'm trying to create a component for this is then it will remove all that irrelevant logic from the JS tab. However, I'm having problems, as always, where I want to control the full screen state within the component, but also want to be able to force the component to change state from an external button....

@chandlerprall or @thompsongl Can you help me with this one. I'd really rather not have to control the state externally if at all possible.

src/components/horizontal_rule/_horizontal_rule.scss

kibanamachine · 2020-03-18T15:21:18Z

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

kibanamachine · 2020-03-18T15:52:08Z

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

kibanamachine · 2020-03-18T16:23:27Z

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

Including the addition of the EuiCollapsibleNavToggle component

kibanamachine · 2020-03-18T16:55:50Z

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

cchaos · 2020-03-18T18:00:16Z

With @thompsongl's help ❤️ , This is now ready for review.

src/components/flyout/_flyout.scss

src/global_styling/mixins/_helpers.scss

kibanamachine · 2020-03-18T18:17:49Z

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

snide

Looks pretty clean. Nice work. I know I should have caught this earlier, but for the screen reader stuff it would be nice to use a role group or something that gave the number of items in the various groups [2 of 7]...etc.

snide · 2020-03-19T22:04:43Z

src-docs/src/views/collapsible_nav/collapsible_nav_all.tsx

+              docked={navIsDocked}
+              onClose={() => setNavIsOpen(false)}>
+              {/* Dark deployments section */}
+              <EuiFlexItem grow={false} style={{ flexShrink: 0 }}>


There's a bunch of style tags in here. I know it's just docs, but likely this stuff will just be copy pasted. Should we make some selectors for these things since they'll need to make it anyway?

That's actually good....? Just because they're necessary for the scrolling to work properly...

Sorry, I guess I just meant, none of these looked dynamic. Should we make some actual css selectors rather than style tags, since they seem necessary for the component to work.

Ok so here's what's going on with the style tags. Really the only ones are:

flex-shrink: 0 on EuiFlexItems

maxHeight on the collapsible nav group

Remove both of those and the nav still works it's just all one big scroll. This implementation is a custom one where we (Kibana) wants the pinned section to scroll independently of the rest. They're not necessary for the Collapsible nav to work.

If we add a class for no. 1, It would be some arbitrary .eui-flexShrinkNone which I really don't think we want to go down this utility class path. Or it would be a specific .euiCollapsibleNavGroup__wrapper that simply adds the that one property in which case most consumers won't know why they need it or what it's doing.

We can't add a class for no. 2, because the value is completely dependent on the consumer and honestly just a best guess by me right now.

So really then it's back to, do we really want to add some random class for flex-shrink: 0 when ,honestly, shrink should just be a prop on EuiFlexItem like grow. But I tried adding this and it got a bit complicated so I abandoned.

I was thinking it was more something like euiCollapsableNav__something that just applied that stuff. Either way, your call. Just felt weird copy/pasting style tags.

src-docs/src/views/collapsible_nav/collapsible_nav_list.tsx

src/components/flyout/_flyout.scss

src/components/horizontal_rule/_horizontal_rule.scss

src/components/header/_variables.scss

src/global_styling/mixins/_helpers.scss

cchaos · 2020-03-20T19:25:49Z

for the screen reader stuff it would be nice to use a role group or something that gave the number of items in the various groups [2 of 7]...etc.

@snide I'm not sure where you need this. No matter which component i add it to, it doesn't read out as you want. EuiListGroup's do say the total number of list items per group.

cchaos · 2020-03-20T19:28:20Z

@myasonik This is probably a good time to do an a11y assessment. 🙏

kibanamachine · 2020-03-20T19:47:45Z

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

snide · 2020-03-20T19:48:22Z

@snide I'm not sure where you need this. No matter which component i add it to, it doesn't read out as you want. EuiListGroup's do say the total number of list items per group.

You can make me an issue. I can tackle it at a later time. I always forget how to do it. It's often tied extremely closely to the semantics in the page. It's just a nice to have I always try to apply for a11y whenever we have lists of links.

kibanamachine · 2020-03-20T21:26:43Z

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

myasonik · 2020-03-23T17:01:26Z

Not 100% sure where all feedback should go (whether it's part of this work specifically or not) but I'll put everything I have here and whatever you want can be pulled out into future tickets if you like.

I think this is just a docs change, but just to ensure we have good examples, all the pinnable items should have unique text (e.g., instead of the aria-label reading "Pin item" it should read "Pin Dashboards link")
Again I think this is just a docs change, but we should try for all of the lists to have titles (either aria-label or aria-labelledby). It would be awesome if this was wired up for implementing developers but I can understand how that might be tricky.
- This gets a little hairy if there's more than one child of a collapsible element. If there's more than one child, the lot of them should be wrapped in a role=group with the aria-label or aria-labelledby applied to the group
- It looks like an appropriate aria-label is applied to a div.euiCollapsibleNavGroup. If there's more than one child, adding role=group to this would be perfect. If there's only 1 list as a child, it would be best to not add the role=group and move the aria-label to the list.
I wasn't able to recreate it often but a couple times I had the focus state get lost after click "close" on the flyout. I didn't go digging through the code but if there's isn't explicit focus management there, that would reduce reliance on browsers having to guess where to put focus which may be why I would occasionally stumble here. (After clicking close, focus should go back to the hamburger icon button.)
The button titled "Elastic security" doesn't have a focus state

Happy to hop on a call or something if any of this isn't clear 🙂

- Focus state for accordions without arrow toggles (underline) - Added link name in pin/unpin titles

kibanamachine · 2020-03-24T16:02:07Z

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

snide

From a functionality standpoint, I think this looks pretty good. Code issues were cleaned up or are minor. I think we should consider doing a mobile pass for this component beyond what currently exists. Specifically, the double scrolling interface should probably be dropped on small screens. I think that can be done in a later PR so that we can start on the harder Kibana implementation details first.

cchaos · 2020-03-25T16:42:17Z

Most of the a11y issues have now been addressed with the exception of the toggle button focus.

It's semi-hard to consistently reproduce and to be done properly, would require EuiCollapsibleNav to accept a trigger button then pass back both the trigger and flyout as separate params of children. I'd rather do another PR to address this so it doesn't get lost in this PR which is mostly just docs.

@thompsongl Would you mind doing a quick pass on the code side (at least just for anything in src/)?

kibanamachine · 2020-03-25T16:58:35Z

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

thompsongl

src changes LGTM 👍

* Fixed the passing of `size` from EuiListGroup to items * Fix padding of `EuiCollapsibleNavGroup` if extra action is passed * Reset line-height of heading in button * Fix `title` type for EuiCollapsibleNavGroup * Starting full pattern example * Adjusted EuiFlyout position based on fixed EuiHeader * Utility CSS helper for simple overflow scroll without shadows * Adding GuideFullScreen component * Adding content and storing states * Fixing incompatible type with `href` * Fix EuiHorizontalSizing when in flex groups * Cleanup * Quick fix to nav heading * Using subdued text color * Ghost button in dark section for now * Some browser fixes * Fixes for mobile - Including the addition of the EuiCollapsibleNavToggle component * render prop pattern * clean up * Adding accessibility (?) * One more a11y piece * Addressing some a11y concerns - Focus state for accordions without arrow toggles (underline) - Added link name in pin/unpin titles * More a11y fixes

* [Feature] Added `EuiCollapsibleNav` component (#2977) * [New Nav Feature] Added `ghost` colored EuiListGroupItem (#3018) * [New Nav Feature] Created `EuiCollapsibleGroup` (#3031) * [New Nav Feature] EuiPinnableListGroup (#3061) * [K8 Nav Feature] Added `home` and `menu` glyphs to EuiIcon (#3109) * [New Nav Feature] Final docs examples and patterns (#3117) * [New Nav Feature] Move collapsible nav toggle button to be part of EuiCollapsibleNav (#3168)

* [Feature] Added `EuiCollapsibleNav` component (elastic#2977) * [New Nav Feature] Added `ghost` colored EuiListGroupItem (elastic#3018) * [New Nav Feature] Created `EuiCollapsibleGroup` (elastic#3031) * [New Nav Feature] EuiPinnableListGroup (elastic#3061) * [K8 Nav Feature] Added `home` and `menu` glyphs to EuiIcon (elastic#3109) * [New Nav Feature] Final docs examples and patterns (elastic#3117) * [New Nav Feature] Move collapsible nav toggle button to be part of EuiCollapsibleNav (elastic#3168)

cchaos added the work in progress label Mar 18, 2020

cchaos commented Mar 18, 2020

View reviewed changes

src/components/horizontal_rule/_horizontal_rule.scss Show resolved Hide resolved

cchaos force-pushed the feature/collapsible_nav branch from 43dab28 to e322ded Compare March 18, 2020 15:26

cchaos added 16 commits March 18, 2020 11:27

Fixed the passing of size from EuiListGroup to items

e8c7f8d

Fix padding of EuiCollapsibleNavGroup if extra action is passed

d52b61c

Reset line-height of heading in button

5a88b29

Adding doc example for groups with conetnt

cd2ef91

Adding doc example for groups with conetnt

a33047f

Fix title type for EuiCollapsibleNavGroup

892400a

Starting full pattern example

2dd17b5

Adjusted EuiFlyout position based on fixed EuiHeader

108337b

Utility CSS helper for simple overflow scroll without shadows

d7fd664

Adding GuideFullScreen component

806dc67

Adding content and storing states

7f6a708

Fixing incompatible type with href

7b7ecd0

Fix EuiHorizontalSizing when in flex groups

f41afb4

Cleanup

b16effb

Quick fix to nav heading

0692b9d

Using subdued text color

3974d84

cchaos force-pushed the into-feature/all_together branch from 58da630 to 3974d84 Compare March 18, 2020 15:32

Ghost button in dark section for now

7c03d22

Some browser fixes

1fb6303

Fixes for mobile

3c28b86

Including the addition of the EuiCollapsibleNavToggle component

thompsongl self-requested a review March 18, 2020 16:56

render prop pattern

1940b4d

cchaos marked this pull request as ready for review March 18, 2020 18:00

cchaos removed the work in progress label Mar 18, 2020

cchaos commented Mar 18, 2020

View reviewed changes

src/components/flyout/_flyout.scss Show resolved Hide resolved

src/global_styling/mixins/_helpers.scss Show resolved Hide resolved

cchaos requested a review from elizabetdev March 18, 2020 18:47

snide reviewed Mar 19, 2020

View reviewed changes

cchaos mentioned this pull request Mar 20, 2020

[Feature Branch] EuiCollapsibleNav #3019

Merged

8 tasks

Adding accessibility (?)

ecfc0a1

cchaos requested a review from myasonik March 20, 2020 19:27

One more a11y piece

d2403be

Addressing some a11y concerns

f7c7afe

- Focus state for accordions without arrow toggles (underline) - Added link name in pin/unpin titles

snide approved these changes Mar 24, 2020

View reviewed changes

More a11y fixes

1f27aa7

thompsongl approved these changes Mar 25, 2020

View reviewed changes

cchaos merged commit 2696b02 into elastic:feature/collapsible_nav Mar 25, 2020

cchaos deleted the into-feature/all_together branch March 25, 2020 18:41

cchaos mentioned this pull request Mar 25, 2020

[EuiFocusTrap] Can be funky when it comes to returning focus to originating button #3170

Closed

Conversation

cchaos commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR is primarily docs

1. Example showing different types of children that can be passed to EuiCollapsibleNavGroup

2. Full screen example with header and saved pinning/open/closed states

Checklist

Uh oh!

cchaos commented Mar 18, 2020

Uh oh!

Uh oh!

kibanamachine commented Mar 18, 2020

Uh oh!

kibanamachine commented Mar 18, 2020

Uh oh!

kibanamachine commented Mar 18, 2020

Uh oh!

kibanamachine commented Mar 18, 2020

Uh oh!

cchaos commented Mar 18, 2020

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Mar 18, 2020

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

snide Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

snide Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

snide Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cchaos commented Mar 20, 2020

Uh oh!

cchaos commented Mar 20, 2020

Uh oh!

kibanamachine commented Mar 20, 2020

Uh oh!

snide commented Mar 20, 2020

Uh oh!

kibanamachine commented Mar 20, 2020

Uh oh!

myasonik commented Mar 23, 2020

Uh oh!

kibanamachine commented Mar 24, 2020

Uh oh!

snide 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!

cchaos commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Mar 25, 2020

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

cchaos commented Mar 18, 2020 •

edited

Loading

snide left a comment •

edited

Loading

cchaos commented Mar 25, 2020 •

edited

Loading