-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(DropdownToggle): only set tag prop if the Tag is default button #221
fix(DropdownToggle): only set tag prop if the Tag is default button #221
Conversation
@@ -67,7 +67,7 @@ class DropdownToggle extends React.Component { | |||
), cssModule); | |||
const children = props.children || <span className="sr-only">{ariaLabel}</span>; | |||
|
|||
if (nav) { | |||
if (nav && Tag === Button) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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. |
@@ -42,7 +41,7 @@ class DropdownToggle extends React.Component { | |||
return; | |||
} | |||
|
|||
if (this.props.nav) { | |||
if (this.props.nav && !this.props.tag) { |
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.
added this to align with what was in the Tag
logic.
@TheSharpieOne let me know if this lgtm to you |
LGTM |
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'stag
prop, it would try to set thetag
prop on that thing, which may not support thetag
prop.