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

size props for textField #20477

Closed
mohamedtijani opened this issue Apr 9, 2020 · 8 comments
Closed

size props for textField #20477

mohamedtijani opened this issue Apr 9, 2020 · 8 comments
Labels
component: text field This is the name of the generic UI component, not the React module! discussion
Milestone

Comments

@mohamedtijani
Copy link

mohamedtijani commented Apr 9, 2020

the props size for textField not working

this is the code for 2 textField
the "first name" field with size="small"

<Grid item xs={6}>
              <TextField
                id="outlined-dense"
                label="First name"
                value={this.state.firstName}
                fullWidth
                required
                name="firstName"
                margin="dense"
                variant="outlined"
                onChange={this.handleChangeFirstName}
                helperText={this.state.errorFirstName}
                FormHelperTextProps={{
                  error: true
                }}
                size="small"
              />
            </Grid>
            <Grid item xs={6}>
              <TextField
                id="outlined-dense"
                label="Last Name"
                name="lastName"
                value={this.state.lastName}
                fullWidth
                required
                margin="dense"
                variant="outlined"
                onChange={this.handleChangeLastName}
                helperText={this.state.errorLastName}
                FormHelperTextProps={{
                  error: true
                }}
              />
            </Grid>

Capture d’écran 2020-04-09 à 11 09 47 AM

@eps1lon
Copy link
Member

eps1lon commented Apr 9, 2020

size seems to be an alias for margin. Apparently it was added in #18624 to fix an issue in Autocomplete. But this shouldn't require an alias in TextField. This makes the component needlessly complex. I didn't look at the PR at the time because neither title nor labels indicated that this affected other components.

@eps1lon eps1lon added component: text field This is the name of the generic UI component, not the React module! discussion labels Apr 9, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 9, 2020

@mohamedtijani Make sure you update to the latest version.
@eps1lon The size and margin props have different behaviors. The margin prop extends the size prop with an extra margin. I think that we should consider to remove margin in v5.

@eps1lon
Copy link
Member

eps1lon commented Apr 9, 2020

This needs definitely a critical review

https://github.com/mui-org/material-ui/blob/964f9abc7f0838c83561bf581235742c6cdee721/packages/material-ui/src/FormControl/FormControl.js#L169

This looks not ok.

Same applies to the textfield demos. They fail to illustrate the difference between margin and size.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 9, 2020

@eps1lon What solution would you propose for v5? What do you think of keeping the size demo and updating the layout demo to use something else than the margin prop (removing the prop)?

@eps1lon
Copy link
Member

eps1lon commented Apr 9, 2020

I need to dive into this first. It isn't clear what problem TextField#size is trying to solve. In any case having a separate size and layout demo makes little sense. These concerns are the same.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 9, 2020

@eps1lon There is definitely an overlap. I believe the inputs can be used in a wide variety of contexts. For instance here a small text field in Google Docs to leave a comment:

Capture d’écran 2020-04-09 à 13 57 33

Benchmarking with Bootstrap is interesting: https://getbootstrap.com/docs/4.4/components/forms/#horizontal-form-label-sizing.

Maybe we still need this margin prop. What if we changed the implementation to have these two props isolated? Each with their dedicated impact? Meaning developers could use:

  • the size prop to change the density of the TextField.
  • the margin prop as an alternative to Box, Grid, theme.spacing() or Stack.

@oliviertassinari oliviertassinari added this to the v5 milestone Apr 9, 2020
@mohamedtijani
Copy link
Author

mohamedtijani commented Apr 9, 2020

the problem is resolved
my previous version is 4.0.2, i upgrade it to the last version
thanks

@oliviertassinari
Copy link
Member

I believe we have solved the issue: https://next.material-ui.com/components/text-fields/#sizes.

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

No branches or pull requests

3 participants