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.

Sign up for GitHub

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

Jump to bottom

[Feature] Added EuiCollapsibleNav component #2977

Merged
merged 7 commits into from
Mar 9, 2020
Merged

[Feature] Added EuiCollapsibleNav component #2977

merged 7 commits into from
Mar 9, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Mar 3, 2020 •
edited
Loading

Feature branch

This is the primary container for the new EuiCollapsibleNav component. It doesn't do much but

  • extend EuiFlyout,
  • push to the left side,
  • add docked ability, and
  • setup some responsive behavior

Basic flyout behavior

Screen Recording 2020-03-03 at 06 40 PM

Responsive behavior with docking

Screen Recording 2020-03-03 at 06 40 PM

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 via the Feature Branch

@cchaos
Copy link
Contributor Author

cchaos commented Mar 3, 2020

@myasonik What kind of aria-alert or whatever do you think we will need for when the user initiates the opening of the navigation drawer?

@thompsongl Is there any way to add tests for the responsive behavior? Can it mock setting the window to a certain size?

@kibanamachine
Copy link

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

snide
snide reviewed Mar 4, 2020
View reviewed changes
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Some smaller stuff I noticed. Couple questions.

src-docs/src/views/collapsible_nav/collapsible_nav_example.js Show resolved Hide resolved
src-docs/src/views/collapsible_nav/collapsible_nav_example.js Outdated Show resolved Hide resolved
src/components/collapsible_nav/_collapsible_nav.scss Outdated Show resolved Hide resolved
src/components/collapsible_nav/collapsible_nav.tsx Outdated Show resolved Hide resolved
src-docs/src/views/collapsible_nav/collapsible_nav_example.js Outdated Show resolved Hide resolved
src/components/collapsible_nav/_collapsible_nav.scss Show resolved Hide resolved
myasonik
myasonik suggested changes Mar 4, 2020
View reviewed changes
Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

When undocking the nav, I was surprised when the nav was still open. What do you think about closing the nav when undocked?

src/components/collapsible_nav/collapsible_nav.tsx Outdated Show resolved Hide resolved
src/components/collapsible_nav/collapsible_nav.tsx Outdated Show resolved Hide resolved
src/components/collapsible_nav/collapsible_nav.tsx Outdated Show resolved Hide resolved
src/components/collapsible_nav/collapsible_nav.tsx Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor Author

cchaos commented Mar 4, 2020

When undocking the nav, I was surprised when the nav was still open. What do you think about closing the nav when undocked?

The current behavior is how it was working during the Usability tests. We didn't hear any feedback specifically on this interaction so I'm not inclined to change it.

@cchaos
Copy link
Contributor Author

cchaos commented Mar 4, 2020

I should also mention that the example for CollapsibleNav is not complete. It will get fleshed out with all the other inner content, but for now I'm just trying to nail the actual "drawer" part.

@thompsongl
Copy link
Contributor

thompsongl commented Mar 4, 2020 •
edited
Loading

Is there any way to add tests for the responsive behavior? Can it mock setting the window to a certain size?

I'm not sure that any @media queries would get used (jsdom does not actually draw anything), but render changes based on window.innerwidth in JS should get picked with something like

https://gist.github.com/thompsongl/cec1e5be12078ff9e56dc78296b88e27

No longer using EuiFlyout directly
8574f47 
Also added a `close` button
@cchaos
Copy link
Contributor Author

cchaos commented Mar 5, 2020

Ok the component has been updated to not render a EuiFlyout, but just reuses some styles and copies some functionality over. I addressed (most) of the other comments as well.

I also added a visable "close" button (though I supposed we could make it a accessibleOnFocus element instead).

@cchaos cchaos requested review from snide and myasonik March 5, 2020 21:57
@kibanamachine
Copy link

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

@myasonik
Copy link
Contributor

myasonik commented Mar 9, 2020

Is there any way to have the portal render at the start of the document for when rendering the docked version of the nav? Having it at the end of the document, I think, is pretty confusing from a tab order perspective.

snide
snide approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

My issues were addressed and I did a quick code review of the new restructuring. I'd suggest like DataGrid, setting up a PR for the feature branch merge (in draft mode) and then keeping links to these sub PRs in that PR. That way you'll have a nice reference when you need to write your CL.

We don't often use extends, but looks OK here. Mixing might be better so you can pass stuff to it eventually. Very small comment though.

@cchaos
Copy link
Contributor Author

cchaos commented Mar 9, 2020

Is there any way to have the portal render at the start of the document for when rendering the docked version of the nav? Having it at the end of the document, I think, is pretty confusing from a tab order perspective.

The component does not live in a portal. It's DOM will get inserted wherever you place the component. The example in the EUI docs is at the tail end of the DOM structure so that's where it gets rendered.

@cchaos
Copy link
Contributor Author

cchaos commented Mar 9, 2020

I'd suggest setting up a PR for the feature branch merge (in draft mode)

Yep, I have to have new commits in the branch before Github allows me to create a PR, so I need this one in first.

thompsongl
thompsongl approved these changes Mar 9, 2020
View reviewed changes
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; works as expected

@cchaos cchaos changed the title Added EuiCollapsibleNav component [Feature] Added EuiCollapsibleNav component Mar 9, 2020
@cchaos cchaos merged commit 7d7df65 into elastic:feature/collapsible_nav Mar 9, 2020
@cchaos cchaos deleted the into-feature/collapsible_nav/nav branch March 9, 2020 20:19
@cchaos cchaos mentioned this pull request Mar 9, 2020
Merged
8 tasks
@myasonik myasonik mentioned this pull request Mar 10, 2020
Closed
cchaos added a commit that referenced this pull request Mar 17, 2020
@cchaos
[Feature] Added EuiCollapsibleNav component (#2977)
9fb2aab 
* Setting up file structure

* Added EuiFlyout to render, moved to left, and added docking

* mock euioverlaymask

* Better docs for EuiCollapsibleNav

* Cleanup css

* Adding responsive behavior

* No longer using EuiFlyout directly

*  added a `close` button
cchaos added a commit that referenced this pull request Mar 18, 2020
@cchaos
[Feature] Added EuiCollapsibleNav component (#2977)
b98fd5a 
* Setting up file structure

* Added EuiFlyout to render, moved to left, and added docking

* mock euioverlaymask

* Better docs for EuiCollapsibleNav

* Cleanup css

* Adding responsive behavior

* No longer using EuiFlyout directly

*  added a `close` button
cchaos added a commit that referenced this pull request Mar 26, 2020
@cchaos
[Feature] Added EuiCollapsibleNav component (#2977)
adb7eb2 
* Setting up file structure

* Added EuiFlyout to render, moved to left, and added docking

* mock euioverlaymask

* Better docs for EuiCollapsibleNav

* Cleanup css

* Adding responsive behavior

* No longer using EuiFlyout directly

*  added a `close` button
cchaos added a commit that referenced this pull request Mar 27, 2020
@cchaos
[Feature Branch] EuiCollapsibleNav (#3019)
585b9dd 
* [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)
miukimiu pushed a commit to miukimiu/eui that referenced this pull request Mar 30, 2020
@cchaos @miukimiu
[Feature Branch] EuiCollapsibleNav (elastic#3019)
abb6392 
* [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)
@snide snide mentioned this pull request May 15, 2020
Open
35 tasks
@cchaos cchaos mentioned this pull request Sep 23, 2020
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snide snide snide approved these changes

@thompsongl thompsongl thompsongl approved these changes

@myasonik myasonik myasonik approved these changes

@miukimiu miukimiu Awaiting requested review from miukimiu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cchaos @kibanamachine @thompsongl @myasonik @snide