angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.32k stars 6.72k forks source link

feat(FORM FIELD): Expose Material's density config via a public API #23468

Closed kaspnilsson closed 3 years ago

kaspnilsson commented 3 years ago

Feature Description

Make the private density mixin for the form field (and perhaps other components) public, and allow for overriding of the density config that MDC allows overrides for (this is hidden by an abstraction in AngularMaterial)

https://github.com/angular/components/blob/f3e81968d985096a184dd2c0ab10198cf2f22259/src/material-experimental/mdc-form-field/_form-field-density.scss#L45 https://github.com/angular/components/blob/f3e81968d985096a184dd2c0ab10198cf2f22259/src/material-experimental/mdc-form-field/_form-field-density.import.scss#L3

Use Case

There are TONS of situations where it would be helpful to have higher-density form fields and inputs -- I like that Material enforces some level of proportional scaling, but a fixed minimum height for a text input at 40px is not nearly dense enough for applications that are built around a font size smaller than the 16px that is the default for Material.

crisbeto commented 3 years ago

Duplicate of https://github.com/angular/components/issues/4597.

kaspnilsson commented 3 years ago

This is not a dupe of #4597, as this is an issue specifically with the MDC components and #4597 is marked as "fixed in MDC"

crisbeto commented 3 years ago

The idea is to expose the density for all components once we move the MDC ones into stable.

kaspnilsson commented 3 years ago

I'm currently using the MDC components inside of google3 and am having the issue I mentioned in the OP around hidden non-overrideable variables. Will this config be the one exposed via new code within MDC?

crisbeto commented 3 years ago

Regarding the mixin being private, it should actually be exposed under mdc-form-field-density from the material-experimental index: https://github.com/angular/components/blob/master/src/material-experimental/_index.scss#L43. As for the $density-config, I don't have enough context. @devversion do you know why it's set up this way?

kaspnilsson commented 3 years ago

Totally, and that works just fine -- the specific issue here is that using that mixin with a value less than @include mat-mdc-form-field-density(-4); is unsupported:

Error: "mdc-density: height must be between 40px and 56px (inclusive), but received 36px."
   ╷
52 │     $height: mdc-density.prop-value(
   │ ┌────────────^
53 │ │     $density-config: mdc-textfield.$density-config,
54 │ │     $density-scale: $density-scale,
55 │ │     $property-name: height,
56 │ │   );
   │ └───^
   ╵

The values of 40px and 56px come from mdc-textfield.$density-config, which is unoverrideable with this mixin but is overrideable in the mdc-textfield SCSS (it's marked with !default).

If we could supply our own density config instead of using integers with fixed min/max to scale density we wouldn't have this issue.

devversion commented 3 years ago

@kaspnilsson seems reasonable. Though I wonder how you'd achieve higher density in google3 for other MDC-based components @angular/material-experimental exposes? the public mixins exposed by Angular Material always end up relying on a $density-config map that comes from MDC web.

I don't think our API (at least for parameters of the density mixins) foresees a way to overrides the underlying density config. Unless I miss something, you could override the density configurations (for all MDC-based components), using the Sass with keyword. e.g.

@use '@material/textfield' with (
  $density-config: (
    height: (
      default: 56,
      maximum: 56,
      minimum: 30,
    ),
  ),
);

// then import the @angular/material-experimental/mdc-form-field theme afterwards!

is that sufficient for your case? or do you expect something more official that would be supported by all MDC-based components? If so, that seems like a larger feature request.

kaspnilsson commented 3 years ago

is that sufficient for your case? or do you expect something more official that would be supported by all MDC-based components? If so, that seems like a larger feature request.

I think this can be sufficient for my case, yes. We'll have to be careful with our SASS imports to avoid duplicate variable definition, but I think that's OK :)

angular-automatic-lock-bot[bot] commented 2 years ago

This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.