search

material-components / material-components-web-react

Material Components for React (MDC React)
MIT License
2.02k stars 227 forks source link

text-field: dense does not shrink the height #805

Open seriema opened 5 years ago

seriema commented 5 years ago

Adding the dense prop to a text field component doesn't change the visual of the component. The Material guidelines mention it clearly (see "Dense text fields" under "Filled and "Outlined") but the mdc-textfield has deprecated it and it's going to be removed: image https://github.com/material-components/material-components-web/tree/master/packages/mdc-textfield#deprecation-notice

Having that property documented here when it doesn't work is confusing and trying to add it when it's being removed upstream doesn't make sense to me.

moog16 commented 5 years ago

Yes, however the dense class is still available for devs to use. So we still should be supporting it until MDC Web actually removes it. We can however update our docs to reflect that it may/may not be deprecated.

seriema commented 5 years ago

If someone's implementing their own theme using the classes directly it might work for them. Styling Material without the support of the SASS mixins isn't something I was expecting but maybe that's common?

At least if one's using the CSS supplied in this repo (and upstream) then there's no visual difference and that's confusing.

It's not available in the demos which compounds to (my) confusion: https://material-components.github.io/material-components-web-catalog/#/component/text-field

moog16 commented 5 years ago

I'm not quite sure what you mean that there is no visual difference. If you add the prop dense to the textField component, it addes the --dense class, which updates the styles. This can be seen in the stackblitz here.

seriema commented 5 years ago

By looking at these two text-fields, one cannot see which one has dense={true}: image https://stackblitz.com/edit/text-field-dense?file=TextField.js

The font changes slightly on the placeholder label, but the text field height remains the same. dense having the same height isn't what I'd expect. I could write my own CSS, but I haven't needed to do that for other props. So just updating the docs to set the right expectations would help.

moog16 commented 5 years ago

The MDC web team has been talking about deprecating dense altogether. So I'm fine removing dense. We can keep this issue open, however I'm not sure when we will prioritize it. Please feel free to open a PR until then.