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 toggle #1701

Merged
merged 34 commits into from
Mar 12, 2019
Merged

Adds side nav toggle #1701

merged 34 commits into from
Mar 12, 2019

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Mar 8, 2019

Summary

This PR changes EuiNavDrawer from a hover-to-expand to a toggle-to-expand interaction where the latter is done via a fixed bottom button. Along with this, the drawer is now collapsed by default, shows tool tips on hover (optionally), and only allows for one panel to be open at a time (previously both the main and sub menu could be expanded next to each other).

Code re-organization

These changes resulted in a fair bit of code reorganization to simplify the implementation for users, however this code has remained largely the same, functionally speaking. In other words, most of the state and interaction 'guts' lived in the docs example (and in Kibana) and have since been moved into the main EuiNavDrawer component. Additionally, I have rolled the former EuiNavDrawerMenu component into EuiNavDrawer (and removed that component file) since it was essentially just a simple wrapper.

Flyout sub menu generation

The flyout/sub menu has also been moved into EuiNavDrawer and is generated based upon the structure of the listItems data via a new flyoutMenu prop.

Fullscreen docs example

The docs example has also been upgraded to a fullscreen format which better approximates how it will be implemented. Previously, it was in a small envelope that made for some challenging positioning and docs-only style overrides.

Preview

nd

Known issues

  • Not introduced here, we ran into a known bug with focus trap (while building the initial version) that prevents the first item in the sub menu from receiving focus when it initially opens. This means you have to tab through the main nav to reach the flyout nav, so it is navigable but not ideal.

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

@ryankeairns ryankeairns marked this pull request as ready for review March 8, 2019 14:33
@cchaos
Copy link
Contributor

cchaos commented Mar 8, 2019

  1. Do you think we can now do without the show/hide delay and just make the animation ever so slightly longer so it feels smoother?

  1. I get a weird scrollbar issue when opening the secondary menu and then collapsing it

  1. I can't consistently get the secondary menu to collapse when clicking outside of the menu

  1. We should allow the two menus to scroll independently

  1. And we should clean up the transition from going to the secondary menu open to opening the regular menu


@ryankeairns Let me know if you want me to hop in to fix any of the issues above. I haven't looked at the code yet, just did a quick functionality review.

@ryankeairns
Copy link
Contributor Author

ryankeairns commented Mar 8, 2019

@cchaos thanks for the quick review.

I'll dig into 1. removing the delay which I suspect will also address 5. cleaning up animation.

If you could take a look at 4., independent scrolling that would be very helpful. On the topic of scrolling, I can't replicate number 2., perhaps it's our OS differences again.

Regarding 3., I know the clickdetector code is a little wonky... what steps lead you to that state?

@thompsongl
Copy link
Contributor

Not introduced here, we ran into a known bug with focus trap (while building the initial version) that prevents the first item in the sub menu from receiving focus when it initially opens. This means you have to tab through the main nav to reach the flyout nav, so it is navigable but not ideal.

I only see one EuiFocusTrap that surrounds essentially the whole app content area. The lack of focus on the opening menu item would be expected in this case, because there is no new focus trap acting specifically on that new menu context. Or was there a different trap that you tried adding that didn't work out?

@cchaos
Copy link
Contributor

cchaos commented Mar 8, 2019

Regarding 3., I know the clickdetector code is a little wonky... what steps lead you to that state?

Oh I see what's going on, I think, but I was testing it in Chrome's responsive dev tools option, which doesn't have hover (like touch devices) and it looks like you're still closing the sub-menu when hovering outside of it.

What do you think about completely getting rid of all hover to open/close options and just stick to clicking in/out to open/close?

@ryankeairns
Copy link
Contributor Author

ryankeairns commented Mar 8, 2019

Not introduced here, we ran into a known bug with focus trap (while building the initial version) that prevents the first item in the sub menu from receiving focus when it initially opens. This means you have to tab through the main nav to reach the flyout nav, so it is navigable but not ideal.

I only see one EuiFocusTrap that surrounds essentially the whole app content area. The lack of focus on the opening menu item would be expected in this case, because there is no new focus trap acting specifically on that new menu context. Or was there a different trap that you tried adding that didn't work out?

@thompsongl I didn't re-attempt it in this PR, but originally (iirc) it was placed around the flyout sub menu and while it trapped the focus the initial link would not become focused. Chandler helped at the time and I've not since revisited it... it's worth another shot if you're interested :) otherwise it's something we can circle back to after 7.0.x.

@ryankeairns
Copy link
Contributor Author

Regarding 3., I know the clickdetector code is a little wonky... what steps lead you to that state?

Oh I see what's going on, I think, but I was testing it in Chrome's responsive dev tools option, which doesn't have hover (like touch devices) and it looks like you're still closing the sub-menu when hovering outside of it.

What do you think about completely getting rid of all hover to open/close options and just stick to clicking in/out to open/close?

I'm down with that! The hover interactions (I think) have all been removed, so it would just be the onMouseLeave that needs done away with.... I'll check it out.

@thompsongl
Copy link
Contributor

otherwise it's something we can circle back to after 7.0.x.

That's probably best. I think we'd be looking at adding more than one additional EuiFocusTrap, which might hold this PR up.

@cchaos
Copy link
Contributor

cchaos commented Mar 11, 2019

@ryankeairns Can you rebase with master?

…rigger the "now" time selection (elastic#1620)""

This reverts commit d74f18f.
@cchaos
Copy link
Contributor

cchaos commented Mar 11, 2019

FYI, I reverted a bad revert and pushed directly to this branch.

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.

So close. Just a few refactoring issues.

Also, there's a console error when using the extraAction onClick: () => this.expandFlyout(). It is now throwing errors because this.expandFlyout() no longer exists in the src-docs.

src-docs/src/views/nav_drawer/nav_drawer.js Outdated Show resolved Hide resolved
src-docs/src/views/nav_drawer/nav_drawer.js Outdated Show resolved Hide resolved
src/components/list_group/list_group.js Outdated Show resolved Hide resolved
src/components/list_group/list_group_item.js Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.js Outdated Show resolved Hide resolved
Simplify flyout children logic by adding `EuiNavDrawerGroup`
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.

I approve this now with the caveat that the few docs comments I left get updated, the closeFlyout function gets called on clicking the flyout button a second time, and there's a final engineer pass.

😓 Whew. Almost there. Crazy how such a simple looking component can actually be quite complicated.

Great job @ryankeairns !

src-docs/src/views/nav_drawer/nav_drawer_example.js Outdated Show resolved Hide resolved
src-docs/src/views/nav_drawer/nav_drawer_example.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
by applying the common/determined linkItem props directly in the drawer group
@cchaos
Copy link
Contributor

cchaos commented Mar 12, 2019

@ryankeairns I realized that you were declaratively styling how the side nav should look by adding size, style, and aria-label props for every linkItem in the consumer doc. I created a pr that simplifies this for the consumer by applying those "default" props directly inside of the drawer group (though they are still overridable if applied in the object).

This also allowed moving the list item style overrides into it's own folder and applied under it's own SASS file (for readability).

@cchaos
Copy link
Contributor

cchaos commented Mar 12, 2019

Ok I think this PR is ready to roll, if someone wants to do a final quick scan of the code. I just tested in Chrome, FF and IE.

@thompsongl
Copy link
Contributor

@cchaos I'm looking right now 👍

@thompsongl thompsongl self-requested a review March 12, 2019 14:53
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM. Follow up work: #1719

@cchaos cchaos merged commit 29725dc into elastic:master Mar 12, 2019
@ryankeairns
Copy link
Contributor Author

Thanks for getting this merged!!

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.

3 participants