Use 'em' to size TextField elements relatively to fontSize

[thomasguillory]

Currently TextField is using fixed pixel values to size the different
elements composing the component. However, if one wants to change the
size of the component to fit a given design, it's hard to achieve as
there is many sizings to manually set.

This commit is a proposal to size the inner parts of the component
relatively to the fontSize property of its root element, so it's the
only property the user needs to touch.

[oliviertassinari]

@thomasguillory Interesting approach, could you add an example of this in the documentation?
Another way to do it would be to compute values using the fontSize property. I'm wondering what's best.
Also what about styles.floatingLabel.transform = 'perspective(1px) scale(0.75) translate3d(2px, -28px, 0)';?

[thomasguillory]

@oliviertassinari
I missed the transform statement, I'll update that + the documentation.

The advantage of computed values over em might be less inconsistent rounding across browsers.

Also, don't you think that such a modification should be done for every material-ui components and not only TextField ?

[oliviertassinari]

IMO the TextField should be using flexbox. By using absolute and relative values, we have to set a lot of imperative position.
I also think that px is better that em for the same reason that avoiding nested class declaration is considered as a good practice (px is declarative). However in this case (because we are heavily using relative / absolute position) I'm up for this change 👍.

Also, don't you think that such a modification should be done for every material-ui components and not only TextField?

I would say no. Do you have a specific component in mind?

[thomasguillory]

@oliviertassinari I did the missing changes.

There is however one gotcha: _handleTextAreaHeightChange method needs px to em conversion. I did it relying on fontSize. However, if the user pass anything else than a pixel value in this.props.style.fontSize, it won't compute the value correctly.

We would have the same problem if we decided to compute everything using this.props.style.fontSize. Do you have a solution to that?

[oliviertassinari]

However, if the user pass anything else than a pixel value in this.props.style.fontSize, it won't compute the value correctly.

I would say that right now, people are not using the fontSize style property since this TextField is not supporting it correctly.
I think that we should warn them if they do so.

@shaurya947 Are you ok to use em instead of px here?

[shaurya947]

@thomasguillory @oliviertassinari if I understand correctly, what we're proposing is that the user would need to pass in a px value to the style.fontSize prop, and all underlying computation would be done using em (so that things can be scaled relative to the font size)?

If that is the case, I'm good with that. I think most people are already using px for such props so it shouldn't bother them. Yes, we can display a warning if they use another unit.

[oliviertassinari]

Could we add a warning in the dev environment if the fontSize is not a number?
We could use some things like babel-plugin-dev-expression with warning.

[celiao]

Would you mind showing a more explicit example?

[oliviertassinari]

@celiao We already have an example. IMO it's fine. What's missing?

[thomasguillory]

@oliviertassinari warning added, but without babel-plugin-dev-expression, as you have seen here #2107

[oliviertassinari]

@thomasguillory Awesome. I will test it as soon as I have some time.

[oliviertassinari]

if (process.env.NODE_ENV !== 'production') { is not needed anymore. It will be automaticaly added by the babel plugin. Could you remove it?

[oliviertassinari]

I have just tried your PR. It seems that the height of the HintText changed and that the Floating Label is too high on the example you added. I feel like using px over em will be simpler to follow really closely the spec (that use dp).
But on the other hand, we have this #433.

[mbrookes]

An interesting approach. But given the subsequent and planned changes around styling, I'm closing for now. We can reopen if there's still merit.