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

[Select] Remove the React.cloneElement usage #14943

Open
bgits opened this issue Mar 18, 2019 · 10 comments
Open

[Select] Remove the React.cloneElement usage #14943

bgits opened this issue Mar 18, 2019 · 10 comments
Labels
component: select This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@bgits
Copy link

bgits commented Mar 18, 2019

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

This comment has been minimized.

@bgits

This comment has been minimized.

@joshwooding

This comment has been minimized.

@vitkon

This comment has been minimized.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 19, 2019

@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 oliviertassinari added new feature New feature or request component: select This is the name of the generic UI component, not the React module! labels Mar 19, 2019
@oliviertassinari oliviertassinari changed the title TextField does not blur when MenuItem is clicked [Select] Remove the React.cloneElement usage? Mar 19, 2019
@bgits
Copy link
Author

bgits commented Mar 19, 2019

@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
Copy link
Member

@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
Copy link
Member

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

@eps1lon
Copy link
Member

eps1lon commented Jun 25, 2019

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.

@oliviertassinari oliviertassinari changed the title [Select] Remove the React.cloneElement usage? [Select] Remove the React.cloneElement usage Oct 31, 2019
@CarlosAmaral
Copy link

I'm facing the same problem here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants