fix(DropdownToggle): only set tag prop if the Tag is default button#221
Conversation
| const children = props.children || <span className="sr-only">{ariaLabel}</span>; | ||
|
|
||
| if (nav) { | ||
| if (nav && Tag === Button) { |
There was a problem hiding this comment.
Checking if the tag is the default... but what if set by the user and was another reactstrap component which could accept tag... should this code block be ran?
I was thinking about checking if Tag was a function and whether or not Tag.propTypes.tag was defined or something to see if it could accept this prop.
Let me know if that is something that is desired or if just checking that Tag is the default is good enough.
Also, not sure what happens to propTypes in production mode... they might get stripped out since they are only really used for development purposes. (there are things like babel-plugin-transform-react-remove-prop-types)
There was a problem hiding this comment.
I would eventually like to use an env var similar to react to remove proptypes in production code since they aren't used.
I was thinking about checking if Tag was a function and whether or not Tag.propTypes.tag was defined or something to see if it could accept this prop.
To avoid the need for that check I think we should switch the <Tag ... /> to <Button ... tag={UserTag} />
For components that kind of extend an existing component, like this one does with Button, I think the render method should always use the extended component and pass the tag prop along to that component. This way we never pass a reactstrap specific prop to a non reactstrap component.
// user renders:
<DropdownToggler color="danger" tag="span" />
// DropdownToggler render method
<Button {...extraStuff} color="danger" tag="span" />
// Button render method
<span className="..." />I think on paper it's good but I haven't tried it just yet. If it seems reasonable could you update this component in that fashion?
|
@bskimball just curious, what was your usecase for passing a tag prop to DropdownToggler? Custom component? A work around for the time being could be a custom component that removes the tag prop in it's (final) render method. |
TheSharpieOne
force-pushed
the
bugfix/dropdown-toggle-tag-prop-warning
branch
from
7bf1a19 to
cc126c4
Compare
|
Alright, I updated the PR to always use |
|
@eddywashere I was using the dropdown in the navbar and I didn't want it to look like a button. I used just the nav prop at first, but it showed as a button. I then added tag='a' which looked good and worked well, except it threw React's unknown prop error. Using the tag='a' without the nav prop did not look correct either (because the nav-link class is missing). On my project I am using the component I submitted in pull request #220 which evaluates the tag prop in the componentWillMount method. |
|
ran this over the weekend, now I see what the problem is. Thanks for context @bskimball. My bad for the confusion @TheSharpieOne, I think I misread earlier bootstrap docs re: button in nav and maybe button works but definitely the dropdown links should not have |
|
Updated with what was probably in the original pr. |
| } | ||
|
|
||
| if (this.props.nav) { | ||
| if (this.props.nav && !this.props.tag) { |
There was a problem hiding this comment.
added this to align with what was in the Tag logic.
|
@TheSharpieOne let me know if this lgtm to you |
|
LGTM |
3 participants
Supersedes #220
This is one of those interesting 'defaults' which in the case of
nav, it would tell the Button to become an anchor tag, but if the tag was already being override via DropdownToggle'stagprop, it would try to set thetagprop on that thing, which may not support thetagprop.