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

[TextField] Improve dense height to better match the specification #16087

Merged
merged 5 commits into from
Jun 8, 2019
Merged

[TextField] Improve dense height to better match the specification #16087

merged 5 commits into from
Jun 8, 2019

Conversation

Ritorna
Copy link
Contributor

@Ritorna Ritorna commented Jun 6, 2019

The dense variant of the Filled and Outlined Inputs do not follow the material specification.

Expected Behavior 🤔

Heights should be as described in the specification.

see the following sandbox https://codesandbox.io/s/material-demo-co7nd

Current Behavior 😯

image

Context 🔦

We want to build some more complex dialogs using a lot of input components while following the current material specification.

Notes

This PR doesn't support the dense, non-label filled input.

Improve #14521
Closes #15821

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 6, 2019

Details of bundle changes.

Comparing: 3114445...d154bd1

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.05% 🔺 +0.03% 🔺 319,014 319,180 87,423 87,452
@material-ui/core/Paper 0.00% 0.00% 67,964 67,964 20,210 20,210
@material-ui/core/Paper.esm 0.00% 0.00% 61,258 61,258 19,018 19,018
@material-ui/core/Popper 0.00% 0.00% 28,728 28,728 10,344 10,344
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,373 2,373
@material-ui/core/TrapFocus 0.00% 0.00% 3,755 3,755 1,572 1,572
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,016 16,016 5,820 5,820
@material-ui/core/useMediaQuery 0.00% 0.00% 2,529 2,529 1,087 1,087
@material-ui/lab 0.00% 0.00% 138,362 138,362 42,534 42,534
@material-ui/styles 0.00% 0.00% 51,384 51,384 15,186 15,186
@material-ui/system 0.00% 0.00% 14,463 14,463 4,179 4,179
Button 0.00% 0.00% 83,893 83,893 25,499 25,499
Modal 0.00% 0.00% 20,343 20,343 6,687 6,687
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 55,089 55,089 13,848 13,848
docs.main 0.00% +0.01% 🔺 653,440 653,471 206,645 206,656
packages/material-ui/build/umd/material-ui.production.min.js +0.06% 🔺 +0.03% 🔺 292,614 292,780 83,384 83,413

Generated by 🚫 dangerJS against d154bd1

@eps1lon eps1lon added component: Input design: material This is about Material Design, please involve a visual or UX designer in the process labels Jun 6, 2019
@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2019

Looks good but I can't spot a difference for filled TextFields

@Ritorna
Copy link
Contributor Author

Ritorna commented Jun 6, 2019

Looks good but I can't spot a difference for filled TextFields

Yeah haven't had time to implement the necessary changes for FilledInput while being at work. Maybe we could treat them as different PRs.

@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2019

Yeah haven't had time to implement the necessary changes for FilledInput while being at work. Maybe we could treat them as different PRs.

Fine with me. Filled shouldn't be part of PR title and description in that case.

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Jun 6, 2019
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for raising this problem to our attention, it's a great problem to work on! I have linked the related issue. I also believe that we should solve #15821 at the same time. It's deeply correlated.
Will you have the time to push it further?

packages/material-ui/src/OutlinedInput/OutlinedInput.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari changed the title [OutlinedInput, FilledInput] - Height does not match specification when no label is given [TextField] Fix dense height to match the specification Jun 6, 2019
@oliviertassinari oliviertassinari self-assigned this Jun 7, 2019
@oliviertassinari oliviertassinari changed the title [TextField] Fix dense height to match the specification [TextField] Improve dense height to better match the specification Jun 7, 2019
@oliviertassinari oliviertassinari removed their assignment Jun 7, 2019
@eps1lon eps1lon merged commit 562c3ae into mui:master Jun 8, 2019
@eps1lon
Copy link
Member

eps1lon commented Jun 8, 2019

@Ritorna Very nice, thanks!

@TidyIQ
Copy link
Contributor

TidyIQ commented Jun 12, 2019

The new margin="dense" is far too dense now. It looked so much better in the previous version.

@TidyIQ
Copy link
Contributor

TidyIQ commented Jun 12, 2019

Also, the margin being so dense now causes issues if you have an IconButton adornment (e.g. a show/hide password icon button). Since the icon button is larger than the input field it causes the password fields to have a larger height than all other input fields.

Previous version - no issues

This version - issues with IconButton adornments

@eps1lon
Copy link
Member

eps1lon commented Jun 12, 2019

@TidyIQ Please open a separate version and fill out the template.

@oliviertassinari
Copy link
Member

@TidyIQ What you are describing seem expected. The dense variant now follows the specification. The icon button adornment should be dense too (it's not in your example).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design: material This is about Material Design, please involve a visual or UX designer in the process priority: important This change can make a difference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent input height
5 participants