Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds side nav component (EuiNavDrawer) #1427

Merged
merged 22 commits into from
Jan 16, 2019

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Jan 11, 2019

Summary

Adds a component named EuiNavDrawer that wraps one or more EuiListGroup components into an interactive side nav. It is designed to be used in conjunction with EuiHeader and, relatedly, resulted in a couple of minor changes to the EuiHeader design, most notably shortening its height from 64 to 48 pixels.

Note about keyboard accessibility

While keyboard accessibility is possible, it would be improved by using focus-trap for the sub menu that slides out. I attempted to get this working and while I could trap the focus in the flyout/sub menu, I could not get it to return focus to the triggering element upon pressing the esc key.

Long story short, I could use some help sorting this out.

To-do items

  • Make nav collapse when you tab out of the last link
  • Get focus-trap to work properly on the flyout menu, including ability to ESC back to the triggering element (see previous note)

Recordings

Interaction on desktop

nav-drawer

Interaction on mobile

nav-drawer-mobile

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just after a quick glance, it's looking really good, just some issues around responsive and scrolling.


The header buttons with notifications need to be modified so the notification badge doesn't cover the icon this much.

screen shot 2019-01-11 at 14 43 13 pm


At the small breakpoint, there's still a size issue

screen shot 2019-01-11 at 14 45 11 pm


The secondary drawer should scroll independently


It's weird that on mobile I can see the full width of the buttons when I click on them


On mobile it would be nice to be able to use the overlay so that clicking outside of the nav will close it.

src/components/nav_drawer/_nav_drawer.scss Outdated Show resolved Hide resolved
src/components/nav_drawer/_nav_drawer.scss Outdated Show resolved Hide resolved
@ryankeairns
Copy link
Contributor Author

ryankeairns commented Jan 11, 2019

Pushed up changes for:

  • placement of badge count on profile
  • top position of nav drawer on 'small' breakpoint
  • size of button click area on mobile

Still need to do:

  • Use EuiOutsideClickDetector to close menu on mobile when tap outside drawer
  • See what it might take to accomplish dual scroll. As it stands, there is an underlying premise that the whole nav drawer is one piece, so this would potentially require some significant re-tooling. Might need to hold off on that one for now.

@chandlerprall
Copy link
Contributor

  • tabbing through the drawer selects the icons on list items (e.g. Favorites's arrow, Canvas's pushpin) , those icons should probably have tab-index=0
  • tabbing out of of the drawer should collapse the drawer

@ryankeairns
Copy link
Contributor Author

ryankeairns commented Jan 14, 2019

  • tabbing through the drawer selects the icons on list items (e.g. Favorites's arrow, Canvas's pushpin) , those icons should probably have tab-index=0

@chandlerprall Since icons are wrapped in button elements, serving as secondary actions separate from the main button, do the svg icons need the tab-index=0? ...or perhaps I'm not following your comment :)

@alexfrancoeur
Copy link

Closes: elastic/kibana#25736 and potentially elastic/kibana#25741

@ryankeairns
Copy link
Contributor Author

@chandlerprall The drawer will now collapse upon tabbing out.

If you have some time to check out the focus-trap bit, that would be much appreciated. I think that is the last piece to the puzzle here in terms of interaction work. Thanks!

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Ran through the interaction quickly again and the only thing that sticks out is that since the overflow scroll property is added only when it is opened, you'll probably want to figure out a way to auto-scroll the contents back up when the nav drawer re-opens or else this happens:

src/components/nav_drawer/_nav_drawer.scss Outdated Show resolved Hide resolved
src/components/nav_drawer/_variables.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo! Congrats! Let's get this sucker in and we will probably need a follow up after the stab at putting it into Kibana with possible unknowns. Unless @chandlerprall sees anything glaring.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@snide
Copy link
Contributor

snide commented Jan 16, 2019

THIS IS EXCITING

@alexfrancoeur
Copy link

alexfrancoeur commented Jan 16, 2019

image

@ryankeairns
Copy link
Contributor Author

The focus-trap component introduces a hiccup in the animation when expanding the flyout. Looking into that, then will merge!

@ryankeairns ryankeairns merged commit 1a9890e into elastic:master Jan 16, 2019
@ryankeairns
Copy link
Contributor Author

tenor-122497713

@cchaos
Copy link
Contributor

cchaos commented Jan 16, 2019

Haha, I just realized there's no changelog though 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants