-
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
Allow custom components to be rendered in EuiSideNav #310
Allow custom components to be rendered in EuiSideNav #310
Conversation
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.
Nice improvement Rory! I have one idea to pass by you.
</div>); | ||
|
||
const component = render( | ||
<EuiSideNav items={sideNav} component={MyNavButton} /> |
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'm on the fence on this idea but I'd like to know what you think. Would it be better, worse, or neutral if we passed in a renderItem
callback instead? From an interface standpoint, I think this is slightly better because then there's cohesion between the items
prop and the renderItem
prop. It wouldn't change anything structurally:
const renderItem = ({ href, className, children }) => (
<div data-test-id="my-custom-element" href={href} className={className}>
{children}
</div>
);
const component = render(
<EuiSideNav items={sideNav} renderItem={renderItem} />
);
Internally, the implementation would have to change somewhat. I don't think we'd want to set EuiSideNavButton
as the default prop value any more, since the way that's consumed would be different than consuming the callback. What do you think?
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.
@cjcenizal I've pushed a change, let me know what you think.
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.
🏹 Nice! LGTM!
In
EuiSideNav
, allow a component prop to be passed that will be used to render the individual items in the navigation. This makes interoperability with e.g.react-router
easier. The current implementation is difficult to use in Cloud's UI. To make the implementation clean, I refactored the current link / button rendering into a component, and use it by default.Other changes:
side_nav.test.js