Allow SelectMenu.Item to be a or button#787
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/lwuvl5dnp |
dmarcey
left a comment
dmarcey
left a comment
There was a problem hiding this comment.
I played around with it a little on the doc site and it looks good!
Thanks for the quick turn-around.
index.d.ts
Outdated
|
|
||
| export interface SelectMenuItemProps extends Omit<CommonProps, 'as'>, | ||
| export interface SelectMenuItemProps extends CommonProps, | ||
| Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, 'color'> { |
There was a problem hiding this comment.
I wonder if this needs to take into account that this could be an <HTMLButtonElement> too? The as prop is a little weird with TS to begin with so it's probably ok to leave how it is - if we hit issues we can dig deeper - I don't really know what the correct answer would be off the top of my head.
There was a problem hiding this comment.
@smockle this is the definition we're trying to figure out!
There was a problem hiding this comment.
Opened #794 to explore this. Details in that PR description. I’d like to test the new types before we merge—I’ll follow-up here by EOD today.
There was a problem hiding this comment.
The does not allow the "as" prop on SelectMenu.Item test (SelectMenu.js#L77-L85) is failing. The test ought to be updated in light of this PR, right? If I understand correctly, we now want to allow "as" on SelectMenu.Item.
3 participants
This PR allows the
SelectMenu.Itemcomponent to have access to theasprop so that users can make it abuttonif needed. @dmarcey made a good point that sometimes you might want a button instead of an anchor tag as a select menu item, since not all actions taken in a select menu have anhref. More info on links vs buttons in SPAs hereI considered rewriting the type definition to only allow a
buttonoratag, but I figured there might be some cases where someone might need to pass in a styled component to theastag (even though generally we recommend against this).Screenshots
Please provide before/after screenshots for any visual changes
Merge checklist
index.d.ts) if necessaryTake a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.