fix(SideNavItems): unrecognized prop warning for 'isSideNavExpanded' #4627
Conversation
ghost
requested review from
|
Hi there would you mind changing your commit to prop commit convention. TYIA |
dariusbakunas
changed the title
dariusbakunas
changed the title
dariusbakunas
changed the title
|
Deploy preview for the-carbon-components ready! Built with commit e18f9db https://deploy-preview-4627--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit e18f9db https://deploy-preview-4627--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit e18f9db |
|
Deploy preview for carbon-elements ready! Built with commit 53938a4 |
|
Deploy preview for carbon-components-react ready! Built with commit 53938a4 https://deploy-preview-4627--carbon-components-react.netlify.com |
|
Deploy preview for the-carbon-components ready! Built with commit 53938a4 https://deploy-preview-4627--the-carbon-components.netlify.com |
| @@ -19,6 +20,12 @@ const SideNavItems = ({ | |||
| }) => { | |||
| const className = cx([`${prefix}--side-nav__items`], customClassName); | |||
| const childrenWithExpandedState = React.Children.map(children, child => { | |||
| if ( | |||
| typeof child.type === 'string' || | |||
| (child.type === SideNavLink && child.props.element === 'a') | |||
There was a problem hiding this comment.
Hi 👋 thank you for another try - Would you try investigating more wrt what components get isSideNavExpanded prop as a child of <SideNavItems>?
There was a problem hiding this comment.
From all the components that have isSideNavExpanded property (SideNavHeader, SideNavFooter, SideNavLink, SideNavMenu, UIShell/Link) only SideNavLink and UIShell/Link are ones that could potentially pass it to DOM element and produce the warning, I will add another check for UIShell/Link
There was a problem hiding this comment.
is element property used consistently through out the project? If that is the case I could just check if that property value is an a tag and that would handle more scenarios
There was a problem hiding this comment.
Not certain, and I imagine it easily gets no longer the case even if it's the case now. Also, such prop doesn't seem to be in use. BTW wondering why you thought of adding such check...?
There was a problem hiding this comment.
nvm, looks like this is already fixed
|
@dariusbakunas just to clarify, is the use-case here to support elements other than |
| @@ -19,6 +21,13 @@ const SideNavItems = ({ | |||
| }) => { | |||
| const className = cx([`${prefix}--side-nav__items`], customClassName); | |||
| const childrenWithExpandedState = React.Children.map(children, child => { | |||
| if ( | |||
| typeof child.type === 'string' || | |||
There was a problem hiding this comment.
Isn't the typeof the type always string? Maybe should just be child.type.
This sort of type inference is generally super fragile. What happens if someone uses a custom SideNavItem or otherwise wraps the item?
This whole boolean expression is really difficult to parse what the intended behavior is.
There was a problem hiding this comment.
no, type is string only if child is DOM element, not React Component, so if you put <a> tag directly as a child
There was a problem hiding this comment.
I agree, ultimately the issue lies in children that wrap DOM elements, like for example UIShell/Link, that component just passes through all props to React.createElement function.. thus my initial pr had a fix on Link element, I don't think that there is a way to determine at this level that your child is just passing all props to DOM element
There was a problem hiding this comment.
@dariusbakunas just to clarify, is the use-case here to support elements other than
SideNavIteminSideNavItems?
This is to stop React does not recognize the isSideNavExpanded prop on a DOM element warning message when child is SideNavLink with property element set to a
Fixes #3970
Fixes console warning for SideNavItems component:
Changelog
Changed
Avoid passing
isSideNavExpandedproperty to DOM elements or SideNavLink that wraps anchor tagTesting / Reviewing
Launch storybook with
yarn storybookand go tohttp://localhost:9000/?path=/story/ui-shell--sidenav-w-large-side-nav-itemsand observe that console warning about unknown prop is no longer visible