feat: make IconButton polymorphic through 'as' prop #452
Conversation
| * Unfortunately, eslint does not recognize the Polymorphic component has propTypes set | ||
| * | ||
| * We might be able to remove this if we upgrade eslint and associated plugins | ||
| * See: https://github.com/dequelabs/cauldron/issues/451 |
There was a problem hiding this comment.
I started to try upgrading eslint and associated plugins, but we are pretty far out of date. It popped 88 problems that needed to be addressed. I figured it would be better to handle upgrading in a separate PR
packages/react/package.json
Outdated
| @@ -42,6 +42,7 @@ | |||
| "@babel/preset-stage-0": "^7.8.3", | |||
| "@babel/preset-typescript": "^7.9.0", | |||
| "@babel/register": "^7.9.0", | |||
| "@radix-ui/react-polymorphic": "^0.0.14", | |||
There was a problem hiding this comment.
I tried out several libraries and evaluated creating the prop types internally. Other libraries were either unreliable or required code changes to the runtime, which I was trying to avoid. If anything, if we decide not to use this library directly, we should consider copying the code in and citing the source.
There was a problem hiding this comment.
Do we really need a library for this? It seems pretty simple to do on our own.
There was a problem hiding this comment.
I don't mind pulling this in. I have not evaluated segmentio's implementation. The end implementation of a working solution is not overly complicated. However, getting there is pretty finicky. Other implementations I looked at (libraries, blog posts, etc.) either had shortcomings when it came to refs, allowed you to specify as but did not enforce the prop types, or slowed down Typescript a ton.
I went ahead and brought polymorphic in and referenced the source material
| @@ -31,30 +51,31 @@ const IconButton = forwardRef<HTMLButtonElement, IconButtonProps>( | |||
| variant = 'secondary', | |||
| disabled, | |||
| ...other | |||
| }: IconButtonProps, | |||
There was a problem hiding this comment.
Prop types are now defined in PolymorphicIconButton
There was a problem hiding this comment.
I'm a little bit confused by this change, but maybe I'm missing context.
I think for accessibility purposes, a Button should render as a Button, and a Link should render as a link - it's harder to enforce if we're swapping the rendered element under the hood.
We currently support this in <Link> with a variant of button, so I'm wondering if we should allow <Link variant="icon-button"> as well? It's not as straight forward of a change, but I think would be a clearer api by allowing the correct intrinsic underlying html element. I'm requesting changes because I'd like to have a quick discussion before we move forward with this change.
|
For posterity: After jumping on a call and some further back-and-forth, @scurker and I compromised to keep the polymorphic behavior, but add things like |
This is necessary for https://github.com/dequelabs/walnut/issues/2488 to show a "view" icon that links to the new invoice page