MenuItem: Refactor to TypeScript

[bangank36]

What?

Refactor MenuItem component to TypeScript

Why?

Part of #35744, as suggested in #36128 (comment)

How?

• Followed general suggestions from the TypeScript Migration guide.
• Renamed file to .tsx and created a types.ts file with relevant types
• Fix conflicting of MenuItem reference on menu-items-choice and tools-panel-header

Testing Instructions

• Ensure no type error
• Component should work as expected

Testing Instructions for Keyboard

Screenshots or screencast

[mirka]

Great work here, thank you! 🎉

I noticed a couple small things, but this is very close to landing. Aside from that, we'll also need a changelog entry, and we'll need to resolve the merge conflict (looks like a disabled prop was added recently in #52737).

[mirka]

Awesome work here, thank you!

I really enjoyed collaborating with you on these. Looking forward to see you around more 🙌

[bangank36]

I really enjoyed collaborating with you on these. Looking forward to see you around more 🙌

Thank you for taking the time to review these commits, I actually created some other PRs while waiting for the updates
#53379
#53172

Next step would be provide the missing stories for the MenuItem component, FYI I updated the list of missing stories? Though I think it would be wiser to wait for the update on your notes #36128 (comment)

[noahtallen]

Hi @bangank36! We use MenuItem in a couple of places, and I noticed it no longer accepts a couple of Button props:

• variant
• isPressed

Are these supposed to be accepted?

[mirka]

@noahtallen We basically omitted a bunch of Button props we thought were inappropriate in a MenuItem context. Can you point to some use cases for variant/isPressed that you think are legitimate? We're open to reconsider.

[noahtallen]

Good question! This is older code, so I'm not sure exactly how much these props are needed, but here is an example:

https://github.com/Automattic/wp-calypso/blob/5956bfa5df72839c5e29cae607f5c0b152d77147/packages/design-picker/src/components/design-picker-category-filter/index.tsx#L69-L86

Here is where it is rendered:

[noahtallen]

Though it does look like the design is changing slightly after the update:

[mirka]

Yeah, given that the styles are also heavily overridden in that case, it's not a prop that MenuItem will want to officially allow, at least in its current form.

If there's a need for a specific state/style variant, we can add that to MenuItem in a way that's appropriately styled and named. Otherwise, I think for now you should ts-expect-error and TODO on the Calypso side. (We actually did that in one place, too.)

[bangank36]

given that the styles are also heavily overridden in that case

So do you think MenuItem isSelected is more appropriate in this case @mirka, I also noticed that there is no classname for MenuItem selected state yet

[mirka]

So do you think MenuItem isSelected is more appropriate in this case

We'd have to look into it, but off the top of my head, no. Because I don't think the ARIA Menu pattern has a semantic "selected" state. You'd either use a menuitemradio or menuitemcheckbox and set the state on that.

(Also FYI a DropdownMenu v2 is in the works)

[noahtallen]

Otherwise, I think for now you should ts-expect-error and TODO on the Calypso side

Got it -- I did add a couple of one-off wrapper types inline, but I can remove those. Should I just delete the variant/isPressed props if they don't actually do anything?

[mirka]

Got it -- I did add a couple of one-off wrapper types inline, but I can remove those. Should I just delete the variant/isPressed props if they don't actually do anything?

They will do something, because those props are still passed through to the underlying Button. It's just that we "discouraged" it at the TypeScript level.

[noahtallen]

Good to know! Thanks for the clarification.