mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
94.03k stars 32.3k forks source link

[Select] Remove the React.cloneElement usage #14943

Open bgits opened 5 years ago

bgits commented 5 years ago

I have a form with two text fields. The first text field works as expected in selecting and closing on click, but the second does and not have have gone in to very hacky territory in order to try to get it to work but it does not.

Expected Behavior πŸ€”

The menu option should blur on click

Current Behavior 😯

It stays open and does not trigger a handleChange or handleBlur event in the parent. In the code sandbox it doesn't even open.

Steps to Reproduce πŸ•Ή

Link: https://codesandbox.io/s/create-react-app-forked-s5ouz?file=/src/pages/index.js

Context πŸ”¦

I need wrap the menu in an EnhancedComponent in order to Observe certain fields on the model (async)

Your Environment 🌎

Tech Version
Material-UI v3.6.0
React v16.8.0
Browser chrome 72.0.3626.121
TypeScript no
etc.
oliviertassinari commented 5 years ago

@bgits Your codesandbox crash. I can't use it.

bgits commented 5 years ago

@bgits Your codesandbox crash. I can't use it.

I think it maybe related to the bug based on the console error when clicking. I posted this as well which is the code snippet related. https://spectrum.chat/material-ui/help/textfield-does-not-blur-when-menuitem-is-clicked~58c7e043-7ec2-4bb4-ac97-f36dc4002a02

joshwooding commented 5 years ago

@bgits you aren’t destructuring props in EnhancedMenuItems.

vitkon commented 5 years ago

yep, should be const EnhancedMenuItems = ({ profiles }) => (

oliviertassinari commented 5 years ago

@bgits You can't create an intermediary component. We rely on the React.cloneElement() API:

        <TextField
          className={classes.textField}
          id="delegateProfile"
          name="delegateProfile"
          select
          label="Select Delegate Profile"
          placeholder="Select Delegate Profile"
          margin="normal"
          variant="outlined"
          onChange={console.log}
          onBlur={console.log}
          value={""}
        >
          {profiles.map(profile => (
            <MenuItem
              style={{ display: "flex", alignItems: "center" }}
              key={profile.name}
              value={profile.name}
            >
              {profile.name}
            </MenuItem>
          ))}
        </TextField>
      </div>

https://codesandbox.io/s/wz72o1v365

We are working on the problem. @joshwooding has started to remove the React.cloneElement() requirement for a few of our components. But It's not always feasible, we have to evaluate the bundle size increase vs the simplicity gain it provides to people.

bgits commented 5 years ago

@bgits You can't create an intermediary component. We rely on the React.cloneElement() API:

https://codesandbox.io/s/wz72o1v365

We are working on the problem. @joshwooding has started to remove the React.cloneElement() requirement for a few of our components. But It's not always feasible, we have to evaluate the bundle size increase vs the simplicity gain it provides to people.

So for now the only workaround is to have the data prefetched for each MenuItem and display them as a direct child?

oliviertassinari commented 5 years ago

@bgits Yes. Alternatively, we could provide a different API, it might be our best hope actually. I think that we should wait for more people upvotes and use cases before prioritizing the problem.

oliviertassinari commented 5 years ago

A new constraint to take into account, the usage of cloneElement prevents virtualization: #16364.

eps1lon commented 5 years ago

With a priority on customization we should use context by default. Even if it looks like a overengineered cloneElement for the basic case it prevents so much subtle bugs with custom components.

CarlosAmaral commented 4 months ago

I'm facing the same problem here.