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] Update labelWidth for outline variant if required is updated #15386

Merged

Conversation

dmiller9911
Copy link
Contributor

Currently, if a TextField using outlined for the variant has the required prop update to true the asterisk will overlap with the outline a little bit. This will update the width if required changes to accommodate for the additional spacing.

As a note, I did originally go through to write tests for this, but the offsetWidth was always reporting 0 no matter the label or changes. I assume this is a JSDOM limitation. If anyone has any ideas the best way to test this I am open to adding one.

@mui-pr-bot
Copy link

Details of bundle changes.

Comparing: d1f97dc...27cc4dc

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 312,616 312,618 84,445 84,447
@material-ui/core/Paper 0.00% 0.00% 67,279 67,279 19,968 19,968
@material-ui/core/Paper.esm 0.00% 0.00% 60,640 60,640 18,884 18,884
@material-ui/core/Popper 0.00% 0.00% 34,907 34,907 11,812 11,812
@material-ui/core/Textarea 0.00% 0.00% 5,327 5,327 2,291 2,291
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,898 15,898 5,771 5,771
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab 0.00% 0.00% 141,612 141,612 42,722 42,722
@material-ui/styles 0.00% 0.00% 50,833 50,833 15,013 15,013
@material-ui/system 0.00% 0.00% 11,765 11,765 3,928 3,928
Button 0.00% 0.00% 85,621 85,621 25,588 25,588
Modal 0.00% 0.00% 79,901 79,901 24,011 24,011
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 50,908 50,908 11,210 11,210
docs.main 0.00% 0.00% 648,993 648,993 202,179 202,179
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 293,909 293,911 82,489 82,492

Generated by 🚫 dangerJS against 27cc4dc

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! labels Apr 17, 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.

We might want to take the label property into account too.

@eps1lon
Copy link
Member

eps1lon commented Apr 18, 2019

We might want to take the label property into account too.

I thought about this before but I think this is an Anti-Pattern. If you change the label associated to an input then the whole semantics of the input change at which point you should reset it i.e. re-mount it.

One case where this would hide bad UX is #15142. Validation information shouldn't change the label but add a helper text. Not sure about a11y here though so I might be wrong.

Maybe add an example for dynamic labels to the docs:

...
   <TextField key={label} label={label} />
...

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 18, 2019

No need to rush. We can delay the label problem to another time. I do agree that keeping the same label should be encouraged. The question we need to answer is. Should we force people not to change it? Changing the DOM node can be a nonoption in some case, e.g. If you still have the focus on the field.

@oliviertassinari oliviertassinari merged commit a494652 into mui:next Apr 18, 2019
@oliviertassinari
Copy link
Member

@dmiller9911 It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

jztang pushed a commit to nyu-ossd-s19/material-ui that referenced this pull request Apr 21, 2019
@Mark-K
Copy link

Mark-K commented Jun 4, 2019

Updating the outline when the label changes is important for the multiple language use case.
The label is changed to reflect the language setting though the semantics of the field remain the same.
For example with Japanese/English the labels for the fields on a login dialog may be
ユーザ名
Username
パスワード
Password
If the dialog opens in Japanese and is switched to English, the Username label overlaps the outline and there is a large gap after the Password label. The reverse is true when English is the initial setting.
As the language selector is on the login dialog, it is important to preserve previously entered values.

As a kludge, the text fields are placed in a form group that is duplicated for each language and displayed based on the selected language. Seems silly to have multiple fields that input the same values just because the label won't display properly.

@eps1lon
Copy link
Member

eps1lon commented Jun 4, 2019

@Mark-K You can use the label as the key which results in a recalculation if the label changes. It's probably best anyway to remount the textfield if the used language changes since e.g. number format changes anyway.

@Mark-K
Copy link

Mark-K commented Jun 4, 2019

@eps1lon Works beautifully Thanks!!

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

Successfully merging this pull request may close these issues.

5 participants