fix: wire up TabMenu.Item onClick handler

[dkordik]

fix: wire up TabMenu.Item onClick handler

Looks like there was an oversight when refactoring TabMenu.Item to TS. Formerly, this prop was just a passthrough prop. In moving from defaultProps to a "real" prop, we never wired up this now explicit prop to the clickable element, so we were just swallowing the click.
1744170#diff-c2c666f90ee1074db732ccbd92d0c57b

Original JS:

const Item = ({ className: customClassName, active, children, ...props }) => {
...
<li className={className} {...props}>
...
Item.defaultProps = {
  className: '',
  active: false,
  onClick: () => {},
};

became this TS:

const Item: React.FC<ItemProps> = ({
  className: customClassName,
  active = false,
  children,
  onClick = () => {}, // <========= new prop
  ...props
}) => {
...
<li className={className} {...props}>

[pixelbandito]

This looks good to me, but I'd like to verify a TS-using consumer doesn't blow up.
Specifically, I've seen some type errors where the consumer was using an onClick event like React.MouseEvent<HTMLButtonElement> and it blew up with RoverUI's intentionally loose type checking.

[dkordik]

This looks good to me, but I'd like to verify a TS-using consumer doesn't blow up.
Specifically, I've seen some type errors where the consumer was using an onClick event like React.MouseEvent<HTMLButtonElement> and it blew up with RoverUI's intentionally loose type checking.

interesting, so they would specify an "HTMLButtonElement" type of the implementation's underlying element? that one in particular would be odd, because that's not what our underlying element is (though I might be getting lost in the example)

are you thinking we'd be better off removing the explicit prop and returning it to its pre-TypeScript passthrough behavior?

[dkordik]

I guess as a consumer you need to care what type the underlying element is, in case you need to do something like stop propagation of a button event. ew.

looks like if a consumer lets TS infer the event type, their event will get the correct underlying element type from TypeScript, but if they were explicitly specifying the wrong underlying type, it will error (which is how it should work)

I think it would be desirable for a consumer to get a new error if they were explicitly specifying the incorrect underlying element type (like HTMLButtonElement when it's actually a HTMLLIElement)

[pixelbandito]

I agree, if a consumer mis-types, throwing an error is cool.

If our TS RoverUI component isn't doing anything with the onClick except passing it through, then the React.HTMLAttributes<HTMLLIElement> that we're extending for our prop types should be sufficient. Does this component achieve anything by destructuring and re-assigning onClick?

This is just bikeshedding at this point, I just want to be able to give good guidance for future TS conversions.

I ❤️ pushing this code to fix the bug.

[dkordik]

upon further discussion, ended up following up and removing the onClick mapping altogether (nice, GitHub has a link to that^)