Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hidden MenuItem in MenuList causes props warning #12468

Closed
2 tasks done
jkrehm opened this issue Aug 10, 2018 · 7 comments
Closed
2 tasks done

Hidden MenuItem in MenuList causes props warning #12468

jkrehm opened this issue Aug 10, 2018 · 7 comments
Labels
duplicate This issue or pull request already exists

Comments

@jkrehm
Copy link

jkrehm commented Aug 10, 2018

When a MenuList has a MenuItem wrapped by Hidden, a warning is generated.

Failed prop type: The following properties are not supported: tabIndex, onFocus. Please remove them.

I believe MenuList sets those properties, so even if I wanted to not set them they would get set.

  • This is a v1.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

A few issues are similar, but not exactly this issue: #12302 #12159 #11467.

Expected Behavior

The MenuItem should display on the correct screen resolution without producing warnings.

Current Behavior

The MenuItem produces warnings.

Steps to Reproduce

Link: https://codesandbox.io/s/y2vnzl1m8v

For example:

<MenuList role="menu">
  <Hidden smUp>
    <MenuItem onClick={() => {}}>
      Menu Item
    </MenuItem>
  </Hidden>
  <MenuItem onClick={() => {}}>
    Menu Item
  </MenuItem>
</MenuList>

Context

I want some menu items to display on small screen sizes. On large screen sizes they will appear as buttons outside the menu.

Your Environment

Tech Version
Material-UI v1.4.3
React v16.3.2
@oliviertassinari oliviertassinari added the duplicate This issue or pull request already exists label Aug 10, 2018
@oliviertassinari
Copy link
Member

Using the render prop API would solve this issue. Right now, the workaround is to use an intermediary component to forward the properties MenuList is injecting into the MenuItem.

@oliviertassinari oliviertassinari removed the duplicate This issue or pull request already exists label Aug 10, 2018
@jkrehm
Copy link
Author

jkrehm commented Aug 10, 2018

I'm not sure I understand what you mean. Do you mean pass it to MenuList as a render prop? Could I trouble you for a simple example of what you mean?

Since it's just a warning it's not the end of the world, but if there's a work-around I would love to use it to cut down on the noise in my console.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 11, 2018

@jkrehm I hope that help:

import React from "react";
import Hidden from "@material-ui/core/Hidden";
import MenuList from "@material-ui/core/MenuList";
import MenuItem from "@material-ui/core/MenuItem";

// Exposes the injected properties.
const Wire = ({ children, ...props }) => children(props);

const Menu = () => (
  // MenuList is using React.cloneElement for all it's children.
  <MenuList role="menu">
    <Wire>
      {props => (
        <Hidden smUp>
          <MenuItem {...props} onClick={() => {}}>
            Menu Item
          </MenuItem>
        </Hidden>
      )}
    </Wire>
    <MenuItem onClick={() => {}}>Menu Item</MenuItem>
  </MenuList>
);

export default Menu;

https://codesandbox.io/s/74v3z864wx

@oliviertassinari oliviertassinari added component: menu This is the name of the generic UI component, not the React module! new feature New feature or request labels Aug 11, 2018
@oliviertassinari
Copy link
Member

Anyway, I think that we can remove the internal cloneElement, it would avoid using the need to introduce a render prop API.

@jkrehm
Copy link
Author

jkrehm commented Aug 11, 2018

Thank you for that example. It's very helpful.

Also, if the internal API can change so this work-around isn't needed, even better.

@oliviertassinari oliviertassinari added duplicate This issue or pull request already exists and removed component: menu This is the name of the generic UI component, not the React module! new feature New feature or request labels Oct 31, 2019
@oliviertassinari
Copy link
Member

I'm closing for #14943. If we solve #14943, we get this one solved too.

@hypnoboutique
Copy link

hypnoboutique commented Jul 31, 2020

import React from "react";

import { MenuItem, MenuList, useMediaQuery, useTheme } from "@material-ui/core";

const Menu = () => {
  const theme = useTheme();
  const smUp = useMediaQuery(theme.breakpoints.up("sm"));

  return (
    <MenuList role="menu">
      {smUp && <MenuItem onClick={() => {}}>Menu Item</MenuItem>}
      <MenuItem onClick={() => {}}>Menu Item</MenuItem>
    </MenuList>
  );
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants