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 Branch] EuiCollapsibleNav #3019

Merged
merged 12 commits into from
Mar 27, 2020
Merged

[Feature Branch] EuiCollapsibleNav #3019

merged 12 commits into from
Mar 27, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Mar 9, 2020 •
edited
Loading

This branch is the culmination of work needed to achieve the new K8 navigation.

PR's:

1. #2977 Added EuiCollapsibleNav component : Added the flyout mechanism and docking ability.
2. #3018 Added ghost colored EuiListGroupItem and color to EuiListGroup, and increased overall size of large EuiListGroupItem
3. #3031 Added EuiCollapsibleNavGroup
4. #3061 Added EuiPinnableListGroup
5. #3117 Final docs examples and patterns
6. #3168 Move collapsible nav toggle button to be part of EuiCollapsibleNav

Consuming app todos:

1. Remove any hacks for flyouts with fixed headers See this comment

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

@kibanamachine
Copy link

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

@cchaos cchaos changed the title [Feature Branch] Added EuiCollapsibleNav component (#2977) [Feature Branch] K8 Navigation (#2977) Mar 9, 2020
@kibanamachine
Copy link

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

2 similar comments
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@snide snide changed the title [Feature Branch] K8 Navigation (#2977) [Feature Branch] 7.8 Navigation (#2977) Mar 24, 2020
@kibanamachine
Copy link

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

@myasonik
Copy link
Contributor

Wasn't sure where else to put this so hopefully this is ok...
Two things that I must have missed earlier:

  • aria-current is being added to a span here and I can't quite work out where the span is coming. aria-current=page should be placed on a link.
  • Small thing, but the label for the unpin button should read Unpin Home. After pinning any other item, their unpin button labels are correct.

Screen Shot 2020-03-25 at 18 37 33

@ryankeairns
Copy link
Contributor

As usual, fantastic work! Just took a quick pass (these auto-generated preview links are 🔥) and noticed one small change that might be missing.

In response to the user testing feedback, I believe @johnbarrierwilson added some weight and darker color to the Home/pinned links to make them more visible. Is that correct John?

Screenshot 2020-03-26 08 07 13

@cchaos
Copy link
Contributor Author

cchaos commented Mar 26, 2020

Sure, that's just a matter of changing the example from color=subdued to color=text

@cchaos
Copy link
Contributor Author

cchaos commented Mar 26, 2020

@myasonik The reason there are spans is because I didn't want to add so many href="#" or basically non-links, so I just didn't pass anything for those causing them to not render as anchor tags. But if it's a big deal to make sure the anchor tags are being rendered, I'll add hrefs.

Also, the home link really shouldn't have a pin option since it's the only place it exists.

@cchaos
Copy link
Contributor Author

cchaos commented Mar 26, 2020

@ryankeairns @myasonik I have addressed those in #3168

@ryankeairns
Copy link
Contributor

@cchaos I might be misinterpreting your comment, but was the intent for #3168 to include the text color change for the pinned items examples?

@cchaos
Copy link
Contributor Author

cchaos commented Mar 26, 2020

Yes, be sure to refresh now that the new build is up. But the pinned section uses the text color instead of subdued:

Screen Shot 2020-03-26 at 11 46 21 AM

@ryankeairns
Copy link
Contributor

Got it! Ye ol' hard refresh fixed it :) Thank you

@cchaos cchaos changed the title [Feature Branch] 7.8 Navigation (#2977) [Feature Branch] 7.8 Navigation Mar 26, 2020
@cchaos cchaos changed the title [Feature Branch] 7.8 Navigation [Feature Branch] 8.0 Navigation Mar 26, 2020
@cchaos cchaos changed the title [Feature Branch] 8.0 Navigation [Feature Branch] EuiCollapsibleNav Mar 26, 2020
cchaos and others added 7 commits March 26, 2020 16:57
@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
[New Nav Feature] Added ghost colored EuiListGroupItem (#3018)
6d7a3b8 
* Added color=ghost to list group item

And fixed class names for list group

* Added `color` prop to EuiListGroup

Fixed color on disabled list group items

* Fixing hover colors for each list item color

* ghost example

* Increase the height of large items too

Fixes the hidden underline in focus state

* Fixing demo to not apply black bg to list item

* Snaps
@cchaos
[New Nav Feature] Created EuiCollapsibleGroup (#3031)
edbcd9d 
* Added `EuiCollapsibleNavGroup` component

* Initial render of nav group

* Adding background color options

* Fixing more colors

* Added `collapsible` prop

* cleanup

* remove slugify

* Added `titleSize` and `titleElement` to groups and better docs

* better docs

* snaps

* doc cleanoup

* Fixing contrast of focus state of dark bg

* specific classname target

* Using EuiTitle and sizing to wrap title

* `collapsible` -> `isCollapsible`

* Fixing `id` as state and exporting proper Prop types
@cchaos @thompsongl
[New Nav Feature] EuiPinnableListGroup (#3061)
351bf4e 
* Added EuiCollapsibleNavGroupList component

* Added content to EuiCollapsibleNavList

* Update to use latest props from EuiListGroup

* Passing `color` to EuiListGroup

* Added active link to examples

* Better docs for nav list and added snippets

* cleanup

* Renamed `EuiCollapsibleList` to `EuiPinnableListGroup` and moved to `list_group`

Also made `onPinClick` required

* i18n

* Fix focus and focus-within states

With IE fallback

* Allowing pin icon title/aria-labels to be custom

Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
@cchaos
[K8 Nav Feature] Added home and menu glyphs to EuiIcon (#3109)
dbe9792
@cchaos
[New Nav Feature] Final docs examples and patterns (#3117)
b432d11 
* 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
@cchaos
[New Nav Feature] Move collapsible nav toggle button to be part of Eu…
ac79449 
…iCollapsibleNav (#3168)

* Move collapsible nav toggle button to be part of EuiCollapsibleNav
* No role group
* Alterations to top links and added `pinnable` prop to pinnable list items
@cchaos
Copy link
Contributor Author

cchaos commented Mar 26, 2020

Pulling this out of draft and working on a changelog. 🎉

@cchaos cchaos marked this pull request as ready for review March 26, 2020 21:01
@cchaos cchaos requested a review from snide March 26, 2020 21:01
@kibanamachine
Copy link

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

snide
snide approved these changes Mar 26, 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.

Did code reviews on previous PRs. I ran a browser and accessibility check as well as double checked all functionality. This is mergible.

The following are extremely small quibles I'd prefer we leave to later PRs.

I think we could probably improve the animation hiccup that happens when you undock.

I think we could improve the mobile experience by removing the dual scroll system and make it scroll against a single list.

@ryankeairns ryankeairns mentioned this pull request Mar 26, 2020
Closed
13 tasks
@kibanamachine
Copy link

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

@cchaos cchaos merged commit 585b9dd into master Mar 27, 2020
@cchaos cchaos deleted the feature/collapsible_nav branch March 27, 2020 00:44
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)
@chandlerprall chandlerprall mentioned this pull request Apr 8, 2020
Merged
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

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 @myasonik @ryankeairns @snide