material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Select] Remove the React.cloneElement usage

Open bgits opened this issue 6 years ago • 10 comments

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.

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

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.

bgits avatar Mar 18 '19 17:03 bgits

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

oliviertassinari avatar Mar 18 '19 18:03 oliviertassinari

@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

bgits avatar Mar 18 '19 19:03 bgits

@bgits you aren’t destructuring props in EnhancedMenuItems.

joshwooding avatar Mar 18 '19 20:03 joshwooding

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

vitkon avatar Mar 18 '19 21:03 vitkon

@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.

oliviertassinari avatar Mar 19 '19 11:03 oliviertassinari

@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?

bgits avatar Mar 19 '19 12:03 bgits

@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 avatar Mar 19 '19 23:03 oliviertassinari

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

oliviertassinari avatar Jun 25 '19 12:06 oliviertassinari

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.

eps1lon avatar Jun 25 '19 12:06 eps1lon

I'm facing the same problem here.

CarlosAmaral avatar Jun 27 '24 10:06 CarlosAmaral