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
UnderlineNav fixes #134
UnderlineNav fixes #134
Conversation
// if this is a react-router NavLink (duck typing!), | ||
// set activeClassName={SELECTED_CLASS} | ||
if (child.type.name === 'NavLink') { | ||
newProps.activeClassName = SELECTED_CLASS | ||
} | ||
return React.cloneElement(child, newProps) | ||
return React.cloneElement(child, {className, ...newProps}) |
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.
This could probably just be:
newProps.className = className
return React.cloneElement(child, newProps)
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 think the shorthand here is fine!
I have one minor concern about the duck typing approach of testing export default function MyComponent(props) {
return <div>{props.children}</div>
} might be minified with mangled function names into: exports=function z(b){return A.createElement('div',{},b.children)} ...in which case the export's Anyway, it works for now, and you can see in the live docs that it works in our production bundle, but it's something to be aware of. |
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! I will keep the class name duck typing in mind when looking through the emotion/system-classname stuff. Not sure if emotion minifies the class names or not!
Good call, @emplums. We might want to set |
This addresses a couple of points in #128:
rendersClass()
testing helper intoutils/testing
.I also got a change to
package-lock.json
when I rannpm install
, so I've included that in this PR. I can remove that commit if there's a problem with it, though.