Join className from container with the one from child element #228
Conversation
| { | ||
| ...props, | ||
| className: isActive ? [className, activeClassName].join(' ') : className, | ||
| className: [className, child.props.className, isActive && activeClassName].join(' '), |
There was a problem hiding this comment.
This isn't right:
> ['asdf', false].join(' ')
'asdf false'Maybe just pull in classnames?
There was a problem hiding this comment.
Ah, it was supposed to be null - missed that, thanks.
Do you reckon it's worth to have another dependency just to join three different classes? 🤔
There was a problem hiding this comment.
Huh, I thought it was a dep in the previous version. I guess it never came up?
I mean, React-Bootstrap pulls in classnames anyway, so it's not an extra dependency net for anyone already using React-Bootstrap. Or really classnames is just so darn common, no?
There was a problem hiding this comment.
classnames is quite common for many React project, although, react-router-bootstrap module isn't restricted to be used with react-bootstrap only (i.e., I started to use it with Blueprint recently and it works like a charm). 😃
I'd keep it as tiny as possible as I don't see an advantage of bringing an additional dependency for such a simple use case 😉
There was a problem hiding this comment.
Blueprint uses classnames too:
https://github.com/palantir/blueprint/search?utf8=%E2%9C%93&q=classnames&type=
Hand-rolling the class joining would be the worst thing. I'd personally use classnames but it's your call – either way is fine.
There was a problem hiding this comment.
GAH sorry typed too quickly, I meant wouldn't be the worst thing. sorry ):
There was a problem hiding this comment.
It does, you're right. 👍
I mentioned Blueprint just as one of the examples that react-router-bootstrap can be used with other libraries (not even libraries but, potentially, custom components).
Anyway, if this comes up at any point later, I'll pull classnames in 😉
Thanks for an instant response and valuable comments!
Fixes #218