Skip to content

[TextField] Remove line-height reset#22149

Merged
oliviertassinari merged 3 commits intomui:nextfrom
imnasnainaec:remove-input-height-reset
Aug 17, 2020
Merged

[TextField] Remove line-height reset#22149
oliviertassinari merged 3 commits intomui:nextfrom
imnasnainaec:remove-input-height-reset

Conversation

@imnasnainaec
Copy link
Contributor

@imnasnainaec imnasnainaec commented Aug 10, 2020

Resolves #22104 , under advisement of @joshwooding

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 10, 2020 •
edited
Loading

Details of bundle changes

Generated by 🚫 dangerJS against 2b9386f

@joshwooding
Copy link
Collaborator

joshwooding commented Aug 10, 2020 •
edited
Loading

Argos Review:

  • Selects - Need to investigate.
  • Multiline - Need to investigate
  • Autocomplete - I think this change makes the text alignment better
  • Validation TextFields - Seems fine
  • AppBar - Seems fine
  • TextFields - Seems fine
  • Table Pagination - Need to investigate

@joshwooding joshwooding changed the title [Input] Remove line-height reset (revert #10346) [Input] Remove line-height reset Aug 11, 2020
@joshwooding joshwooding changed the title [Input] Remove line-height reset [TextField] Remove line-height reset Aug 11, 2020
@joshwooding joshwooding added the scope: text field Changes related to the text field. label Aug 11, 2020
@imnasnainaec
Copy link
Contributor Author

imnasnainaec commented Aug 11, 2020

Is this height inconsistency a problem?
image

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 11, 2020

Is this height inconsistency a problem?

@imnasnainaec Yes, I think so

@imnasnainaec
Copy link
Contributor Author

imnasnainaec commented Aug 11, 2020 •
edited
Loading

@joshwooding @oliviertassinari Would it be reasonable, instead of removing the line-height reset, to set it at 19px for size="small" and 22px for size="normal"?

@joshwooding
Copy link
Collaborator

joshwooding commented Aug 15, 2020

@imnasnainaec It's worth giving it a try, if that doesn't work we should probably revisit it when we look at TextField next, as I'm sure there are changes needed to align with the spec.

@github-actions github-actions bot mentioned this pull request Aug 15, 2020
Closed
@imnasnainaec
Copy link
Contributor Author

imnasnainaec commented Aug 16, 2020

It's not forcing size="normal" any bigger--I think that's good:
Screen Shot 2020-08-15 at 23 26
But it seems some things need a little less padding on bottom:
Screen Shot 2020-08-15 at 23 28

oliviertassinari
oliviertassinari approved these changes Aug 16, 2020
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have tried the following approach:

  • Increase line-height by +4px
  • Reduce padding by -4px

It seems to work great. In hindsight, I was mistaken during the first implementation, the default line-height wasn't 19px but 24px. We are at 23px now.

@oliviertassinari oliviertassinari added the type: bug It doesn't behave as expected. label Aug 16, 2020
@oliviertassinari oliviertassinari added the breaking change Introduces changes that are not backward compatible. label Aug 16, 2020
@oliviertassinari oliviertassinari merged commit 109bb36 into mui:next Aug 17, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 17, 2020

@imnasnainaec Thanks for exploring some of the scope of the issue :)

@imnasnainaec
Copy link
Contributor Author

imnasnainaec commented Aug 17, 2020

@oliviertassinari Thank you for doing the work!

@imnasnainaec imnasnainaec deleted the remove-input-height-reset branch August 17, 2020 22:07
@imnasnainaec imnasnainaec mentioned this pull request Aug 20, 2020
Closed
@eps1lon eps1lon added this to the v5 milestone Sep 15, 2020
@oliviertassinari oliviertassinari mentioned this pull request Sep 15, 2020
Closed
42 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

+1 more reviewer

@joshwooding joshwooding joshwooding approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking change Introduces changes that are not backward compatible. scope: text field Changes related to the text field. type: bug It doesn't behave as expected.

Projects

None yet

Milestone

v5.0.0-beta.0

Development

Successfully merging this pull request may close these issues.

TextField doesn't support diacritic "combining low line" (U+0331) below characters with a tail (g,j,p,q,y).

5 participants

@imnasnainaec @mui-pr-bot @joshwooding @oliviertassinari @eps1lon