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

[New Nav Feature] Created EuiCollapsibleGroup #3031

Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Mar 10, 2020

EuiCollapsibleGroup

This is the accordion style grouping that will make up these three portions:

Screen Shot 2020-03-10 at 13 16 39 PM

The contents of the group doesn't matter, hence the scribbles

Instead of trying to create 3 components, I made one that is child-agnostic and can be an accordion via collapsible=true or not. Consumers can also add a header with title and iconType. They can also choose the background color.

Screen Shot 2020-03-10 at 13 19 31 PM

Accessibility

Since it does wrap the EuiAccordion when collapsible=true, if there's any a11y needs for that specifically, we should do it in EuiAccordion and make an issue for that.

The one thing I had to deal with is the focus states of the dark background accordion toggle arrow were hard to see. So I forced a different focus ring color with overrides.

Screen Shot 2020-03-10 at 15 59 29 PM

Dark mode

In dark mode, they all just get different shades of dark that aren't that noticeable from each other. 🤷‍♀

Screen Shot 2020-03-10 at 13 21 51 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 to feature PR

@myasonik
Copy link
Contributor

...focus states of the dark background accordion toggle arrow are hard to see. Should I force a different color with overrides?

+1

@kibanamachine
Copy link

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

@cchaos cchaos marked this pull request as ready for review March 10, 2020 20:26
@kibanamachine
Copy link

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

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.

Looks simple enough. Did a quick screenreader check and browsed the code. Only bit I noticed was collapsable spitting out some weirdness in the autodocs. Note reads well enough though.

image

Copy link
Contributor Author

@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.

Only bit I noticed was collapsable spitting out some weirdness in the autodocs. Note reads well enough though.

Yeah this happens when a prop is technically a boolean, but depending on it's value requires another prop or extends another type. Hopefully that'll all get fixed via GSOC 😏

type GroupAsAccordion = EuiCollapsibleNavGroupProps &
Omit<EuiAccordionProps, 'id'> & {
/**
* If `true`, wraps children in the body of an accordion,
* using the `title` (required for use with `collapsible`) as the button
*/
collapsible: true;
/**
* The title gets wrapped in the appropriate heading level
* with the option to add an iconType
*/
title: ReactNode;
};
type GroupAsDiv = EuiCollapsibleNavGroupProps & {
/**
* When `false`, simply renders a div without any accordion functionality
*/
collapsible?: false;
/**
* The title gets wrapped in the appropriate heading level
* with the option to add an iconType
*/
title?: ReactNode;
};

@kibanamachine
Copy link

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

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.

Looking good; just a couple requests.

@cchaos cchaos requested a review from thompsongl March 11, 2020 21:05
@kibanamachine
Copy link

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

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!

@cchaos cchaos merged commit 760d8c0 into elastic:feature/collapsible_nav Mar 12, 2020
@cchaos cchaos deleted the into-feature/collapsible_group branch March 12, 2020 15:34
@cchaos cchaos mentioned this pull request Mar 12, 2020
8 tasks
cchaos added a commit that referenced this pull request Mar 17, 2020
* 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 added a commit that referenced this pull request Mar 18, 2020
* 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 added a commit that referenced this pull request Mar 26, 2020
* 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 added a commit that referenced this pull request Mar 27, 2020
* [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)
elizabetdev pushed a commit to elizabetdev/eui that referenced this pull request Mar 30, 2020
* [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)
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