feat(icon-button): support as
#1164
Conversation
| @@ -0,0 +1,11 @@ | |||
| import isPropValid from '@emotion/is-prop-valid'; | |||
There was a problem hiding this comment.
this is used internally by emotion
| {...props} | ||
| as="a" | ||
| href="https://www.kanyetothe.com" | ||
| target="_BLANK" |
There was a problem hiding this comment.
if we don't support passing all valid props, then using regular "external" links would be super annoying.
| return ( | ||
| <AccessibleButton | ||
| buttonAttributes={buttonAttributes} | ||
| as={props.as} | ||
| buttonAttributes={attributes} |
There was a problem hiding this comment.
I have a concern about forwarding all props, given we already "reimplement" some of the HTML props:
- It is possible to pass both the props
disabledandisDisabledto a component, and in this case, because theAccessibleButtonspreads thebuttonAttributesprop after theisDisabledprop is assigned todisabled, which means the native html prop will override theisDisabledone. Same for all other props that we manually assign before thebuttonAttributesspread. Is this an intended behaviour?
There was a problem hiding this comment.
I don't get it. You mean if the user passed in disabled={true} and isDisabled={false}?
There was a problem hiding this comment.
Kind of feels like a play stupid games win stupid prizes situation.
There was a problem hiding this comment.
What would you prefer to happen? I guess we could add a prop-type warning if a user passes both?
There was a problem hiding this comment.
Yeah, it shouldn't happen, but it can lead to weird bugs that are hard to track.
It's more a question of: should we worry about it, and can we do something to mitigate this?
There was a problem hiding this comment.
I would just ignore it. The plan is to deprecate all the isDisabled, etc stuff right?
If someone writes the code
<IconButton disabled={true} isDisabled={false} />
and it passes pull request, then we deserve the bugs.
There was a problem hiding this comment.
Yeah, it totally makes sense to deprecate the isDisabled and others as the next step.
It could happen accidentally after some layers of abstraction, but I also wouldn't worry about it too much, as for now we're still supposed to use the explicit props that are documented.
I'm OK with keeping it as it is, as long as we're aware and we hopefully deprecate those redundant props soon 😇.
There was a problem hiding this comment.
Having two "sources of truth" is a bad idea. 😄
There was a problem hiding this comment.
One could have like a prop-zipper or something so that always one has precedence. However, I think it's a super nice theoretical catch but given the scale we are the practical implications can be negated.
Summary
Adds support for
asto theIconButton.Also supports prop forwarding to
IconButtonandSecondaryButton. Without this, it would not be possible to useanchortags.Closes #1162