[MenuItem] Add buttonRef (and other button props) type

[VincentLanglet]

• I have followed (at least) the PR section of the contributing guide.

I tried to fix this issue #14769

[mui-pr-bot]

No bundle size changes comparing a74b020...1d4c59c

Generated by 🚫 dangerJS against 1d4c59c

[VincentLanglet]

@eps1lon I added the demo

[VincentLanglet]

@eps1lon Hi, what are the next steps to make this PR mergeable ?
Since it fix a bug, I'd like to make it mergeable as quickly as possible 👷

[eps1lon]

Since it fix a bug, I'd like to make it mergeable as quickly as possible

There are still some unaddressed issues which don't look quite right (using ButtonBase over ListItem). I'm not sure if this is an issue with the new approach we're using or if there exists a better solution.

In any case: buttonRef will be deprecated anyway in 4.1 (and discouraged in 4.0). Combined with the fact that this is an alpha and doesn't block the runtime this is a low priority issue for me. I will go over it in the next days but don't expect this to be released with the next alpha.

You could help by making the build pass.

[VincentLanglet]

It's kinda blocking for typescript users since this code does not compile.

function Option(props: OptionProps<OptionType>) {
  return (
    <MenuItem
      buttonRef={props.innerRef}
      selected={props.isFocused}
      component="div"
      style={{
        fontWeight: props.isSelected ? 500 : 400,
      }}
      {...props.innerProps}
    >
      {props.children}
    </MenuItem>
  );
}

You say buttonRef is deprecated, I didn't know. What is the alternative ?
Basically I know nothing about ref. After some try this code works (just removing buttonRef)

function Option(props: OptionProps<OptionType>) {
  return (
    <MenuItem
      selected={props.isFocused}
      component="div"
      style={{
        fontWeight: props.isSelected ? 500 : 400,
      }}
      {...props.innerProps}
    >
      {props.children}
    </MenuItem>
  );
}

Does this do the same thing ?

I dont consider the use of ExtendButtonBase over ListItem like a problem. The generic type is made to say "We add the button props to the ListItem props".

And there is something wrong with the CI for typescript files

diff --git a/docs/src/pages/demos/autocomplete/IntegrationReactSelect.js b/docs/src/pages/demos/autocomplete/IntegrationReactSelect.js
index 0c089c136..16a2f7a0d 100644
--- a/docs/src/pages/demos/autocomplete/IntegrationReactSelect.js
+++ b/docs/src/pages/demos/autocomplete/IntegrationReactSelect.js
@@ -1,7 +1,7 @@
-/* eslint-disable react/prop-types, react/jsx-handler-names */
+/* eslint-disable no-undef, import/named, react/jsx-filename-extension */
+/* eslint-disable react/jsx-no-comment-textnodes, react/jsx-handler-names */
 
 import React from 'react';
-import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import Select from 'react-select';
 import { withStyles } from '@material-ui/core/styles';
@@ -237,6 +237,7 @@ class IntegrationReactSelect extends React.Component {
     return (
       <div className={classes.root}>
         <NoSsr>
+          // @ts-ignore (https://github.com/DefinitelyTyped/DefinitelyTyped/pull/33607)
           <Select
             classes={classes}
             styles={selectStyles}
@@ -248,6 +249,7 @@ class IntegrationReactSelect extends React.Component {
             isClearable
           />
           <div className={classes.divider} />
+          // @ts-ignore (https://github.com/DefinitelyTyped/DefinitelyTyped/pull/33607)
           <Select
             classes={classes}
             styles={selectStyles}
@@ -270,9 +272,4 @@ class IntegrationReactSelect extends React.Component {
   }
 }
 
-IntegrationReactSelect.propTypes = {
-  classes: PropTypes.object.isRequired,
-  theme: PropTypes.object.isRequired,
-};
-
 export default withStyles(styles, { withTheme: true })(IntegrationReactSelect);
Exited with code 1

Removing propTypes, adding // @ts-ignore, ... should be correct.

Anyway, thanks for taking time for me.

[eps1lon]

It's kinda blocking for typescript users since this code does not compile.

You can always @ts-ignore or any stuff. That's why I said it doesn't have high priority. And as usual don't put this into production. We appreciate everybody who tests alpha but those should live in a branch that's not deployed.

[VincentLanglet]

You can always @ts-ignore or any stuff. That's why I said it doesn't have high priority. And as usual don't put this into production. We appreciate everybody who tests alpha but those should live in a branch that's not deployed.

Sure ! But I think it has to be fixed before the release version.

I'm curious... You say buttonRef is deprecated, I didn't know. What is the alternative ?
This ?

const OptionOld = (props: OptionProps<OptionType>) => (
    <MenuItem
      buttonRef={props.innerRef}
      component="div"
      {...props.innerProps}
    >
      {props.children}
    </MenuItem>
  );

const OptionNew = (props: OptionProps<OptionType>) => (
    <MenuItem
      component="div"
      {...props.innerProps}
    >
      {props.children}
    </MenuItem>
  );

[VincentLanglet]

@eps1lon Hi.
I know it's not a big priority for you, but I worked again on this PR and made a lot of different improvement that I think can interest you. If you only want to start merging some of these, I can make a separate PR with the ones you want.

For the MenuItem typing part

For more clarity, I think it's better having MenuItem as an OverridableComponent,
and MenuItemPropsMap = ListItemTypeMap + ButtonBaseTypeMap

declare const MenuItem: OverridableComponent<
  ExtendButtonBaseTypeMap<ListItemTypeMap<{ role?: string }, 'li'>>
>;

For the ListItem typing part

From source code

/**
   * The component used for the root node.
   * Either a string to use a DOM element or a component.
   * By default, it's a `li` when `button` is `false` and a `div` when `button` is `true`.
   */
  component: PropTypes.elementType,

So tests should be

<ListItem
            button
            ref={elem => {
              elem; // $ExpectType HTMLDivElement | null 
            }}
            onClick={e => {
              e; // $ExpectType MouseEvent<HTMLDivElement, MouseEvent>
              log(e);
            }}
          >

And not HTMLButtonElement

ButtonBase.d.ts

The props disabled was missing

formattedTSDemos.js

Fix issue tsx being compiled into js with extra empty lines.

...
<TextField
  required
  id="filled-required"
  label="Required"
  defaultValue="Hello World"
  className={classes.textField}
  margin="normal"
  variant="filled"
/>

<TextField
  error
  id="filled-error"
  label="Error"
  defaultValue="Hello World"
  className={classes.textField}
  margin="normal"
  variant="filled"
/>
...

Babel plugin unwrap createStyles

Unwrap createStyles from @material-ui/styles too

Demo of ReactSelect as tsx file

Compile correctly to the js one

[VincentLanglet]

@oliviertassinari I'll be happy to have your review

[oliviertassinari]

You say buttonRef is deprecated, I didn't know. What is the alternative?

@VincentLanglet The alternative will be the ref property, like with a native <button> element. The related pull request is #13664. We are waiting on facebook/react#15091 outcome to move forward. Well @eps1lon Do we need to wait for their answer? Case A, they merge 🎉 . Case B they don't. I guess we can do a instance of HTMLElement comparison on our side before calling findDOMNode()?

[VincentLanglet]

Since ListItem and MenuItem can have href, it's ExtendButtonBase we have to use.

export interface ExtendButtonBaseTypeMap<M extends OverridableTypeMap> {
  props: ButtonBaseTypeMap['props'] & M['props'];
  defaultComponent: M['defaultComponent'];
  classKey: M['classKey'];
}

export type ExtendButtonBase<M extends OverridableTypeMap> = ((
  props: { href: string } & OverrideProps<ExtendButtonBaseTypeMap<M>, 'a'>,
) => JSX.Element) &
  OverridableComponent<ExtendButtonBaseTypeMap<M>>;

[eps1lon]

@VincentLanglet My main concern resolved around button={false}: 096e185

which is why I insisted on using the root component approach: 1d4c59c

Ideally I would just forward to ListItem but we have reversed default values here so this might be tricky. Any thoughts about this?

[VincentLanglet]

@eps1lon The changes you made seems good to me.