-
Notifications
You must be signed in to change notification settings - Fork 842
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
Add ability to force open EuiSideNav items. #515
Conversation
81843fb
to
1859681
Compare
|
||
expect(component) | ||
.toMatchSnapshot(); | ||
describe('props', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrapped the props
tests in a describe('props')
. Beyond that, I only added the renders items having { forceOpen: true } in open state, and automatically opens parent items
test to this file.
1859681
to
20ea1bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This is fantastic @arkwright. Thanks for doing this. I left a few minor comments but other than that, this PR is good to go.
CHANGELOG.md
Outdated
@@ -16,6 +16,7 @@ instead of just string ([#516](https://github.com/elastic/eui/pull/516)) | |||
- New icons for `logoGithub` and `logoSketch` ([#494](https://github.com/elastic/eui/pull/494)) | |||
- `EuiCard` now has an `href` and `isClickable` prop for better handling hover animations. ([#494](https://github.com/elastic/eui/pull/494)) | |||
- Added `calculateContrast` and `rgbToHex` to services ([#494](https://github.com/elastic/eui/pull/494)) | |||
- Add ability to force `EuiSideNav` items open by setting `item.forceOpen`. ([#497](https://github.com/elastic/eui/pull/515)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the #497
link to #515
?
@@ -34,12 +38,12 @@ export const SideNavExample = { | |||
<p> | |||
<EuiCode>SideNav</EuiCode> is a responsive menu system that usually sits on the left side of a page layout. | |||
It will exapand to the width of its container. This is the menu that is used on the left side of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're here could you fix this typo? "exapand" -> "expand"
mobileTitle: PropTypes.node, | ||
/** | ||
* `items` is an array of objects (navigation menu `item`s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these great docs!!
src/components/side_nav/side_nav.js
Outdated
@@ -40,6 +45,10 @@ export class EuiSideNav extends Component { | |||
...rest | |||
} = item; | |||
|
|||
// Deleted to avoid passing unsupported 'forceOpen' prop to <EuiSideNavItem/>, | |||
// which will result in the unrecognized prop being rendered to the DOM. | |||
delete rest.forceOpen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of deleting, can we destructure this var but then not use it? This is the pattern we've been using elsewhere when we want to prevent props from making their way into rest
.
const {
id,
name,
isSelected,
items: childItems,
icon,
onClick,
href,
forceOpen, // eslint-disable-line no-unused-vars
...rest
} = item;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me from a functional / consumer perspective. Thanks for the PR.
20ea1bd
to
7c325c3
Compare
jenkins test this |
jenkins, test this |
jenkins test this |
7c325c3
to
b3241e8
Compare
b3241e8
to
1f98358
Compare
Fixes #497.
items
passed toEuiSideNav
can now receive anitem.forceOpen
boolean property, which forces that particular nav item (and all of its parents) to render in the "open" state..forceOpen
can be selectively applied to individual items, or can be applied to every item in the tree to force the entire tree to be open. This allows us to draw attention and convenience to certain navigation levels so that they are more easily discoverable by the user.I chose the name
forceOpen
because my first choice,isOpen
, when set toundefined
, shadows theisOpen
prop passed toEuiSideNavItem
, which ironically can force the entire navigation tree closed. For example, if you use acreateItem()
function to create individual navigation items, it might include a line like this:It is impossible to set
isOpen
in the example above using the ternary pattern — all items which do not match will be forced shut, even when selected. Using the propertyforceOpen
avoids this problem, and enables the ternary pattern.Testing
If you want to test this work manually, I added a convenient demo at the bottom of the "Side Nav" living style guide page.