angular / components

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

bug(checkbox): cannot set low density #26045

Closed ghost closed 1 year ago

ghost commented 1 year ago

Is this a regression?

The previous version in which this bug was not present was

No response

Description

In the migration guide, it says that "you can match the previous size by using density -5 for the checkbox." https://material.angular.io/guide/mdc-migration#checkbox

@use '@angular/material' as mat;
@include mat.checkbox-density(-5px);

But when I try to do so, the compiler crashes with error:

./src/styles.scss - Error: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: 40px and -20px*px have incompatible units.
    ╷
111 │     $value: map.get($property-scale-map, default) + $density-scale * $interval;
    │             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
  @material\density\_density.scss 111:13                                   prop-value()
  @material\checkbox\_checkbox-theme.scss 310:11                           get-ripple-size()
  @material\checkbox\_checkbox-theme.scss 324:10                           density()
  node_modules\@angular\material\checkbox\_checkbox-theme.scss 92:7        @content
  node_modules\@angular\material\core\mdc-helpers\_mdc-helpers.scss 216:3  disable-mdc-fallback-declarations()
  node_modules\@angular\material\checkbox\_checkbox-theme.scss 90:3        density()
  src\styles\material.scss 55:1                                            @import
  src\styles.scss 2:9                                                      root stylesheet

If I change -5px to -5 I get another error:

./src/styles.scss - Error: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: "mdc-density: size must be between 28px and 40px (inclusive), but received 20px."
    ╷
310 │     @return density-functions.prop-value(
    │ ┌───────────^
311 │ │     $density-config: $density-config,
312 │ │     $density-scale: $density-scale,
313 │ │     $property-name: size
314 │ │   );
    │ └───^
    ╵
  @material\checkbox\_checkbox-theme.scss 310:11                           get-ripple-size()
  @material\checkbox\_checkbox-theme.scss 324:10                           density()
  node_modules\@angular\material\checkbox\_checkbox-theme.scss 92:7        @content
  node_modules\@angular\material\core\mdc-helpers\_mdc-helpers.scss 216:3  disable-mdc-fallback-declarations()
  node_modules\@angular\material\checkbox\_checkbox-theme.scss 90:3        density()
  src\styles\material.scss 55:1                                            @import
  src\styles.scss 2:9                                                      root stylesheet

Reproduction

In the description

Expected Behavior

It should be possible to match the previous size by using density -5 for the checkbox

Actual Behavior

Setting density below -3 results in a error.

Environment

Angular CLI: 15.0.0
Node: 18.12.0
Package Manager: npm 9.1.1
OS: win32 x64

Angular: 15.0.0
... animations, cdk, cli, common, compiler, compiler-cli, core
... forms, material, platform-browser, platform-browser-dynamic
... router

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1500.0
@angular-devkit/build-angular   15.0.0
@angular-devkit/core            15.0.0
@angular-devkit/schematics      15.0.0
@schematics/angular             15.0.0
rxjs                            7.5.7
typescript                      4.8.4
ghost commented 1 year ago

Workaround (makes the checkbox size 18px, 16px requires more css):

.mat-mdc-checkbox {
  --mdc-checkbox-state-layer-size: 16px;

  .mat-mdc-checkbox-ripple,
  .mdc-checkbox__ripple {
    top: -12px;
    left: -12px;
    right: -12px;
    bottom: -12px;
  }

  label {
    padding-left: 6px !important;
  }
}
crisbeto commented 1 year ago

The density units are just numbers, not pixels. E.g. mat.checkbox-density(-1).

mduft commented 1 year ago

Have the same problem. Following the migration guide is not possible. Also leaving out the 'px' and just passing -5 also does not work:

./src/app-theme.scss - Error: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: "mdc-density: size must be between 28px and 40px (inclusive), but received 20px."
    ╷
310 │     @return density-functions.prop-value(
    │ ┌───────────^
311 │ │     $density-config: $density-config,
312 │ │     $density-scale: $density-scale,
313 │ │     $property-name: size
314 │ │   );
    │ └───^
crisbeto commented 1 year ago

I believe the densities are -2, -1, 0 and minimum. That being said, the error message can be improved.

alan-agius4 commented 1 year ago

Using this mixin also surfaces some sass deprecation warnings.

Deprecation $weight: Passing a number without unit % (60) is deprecated.

To preserve current behavior: $weight * 1%

More info: https://sass-lang.com/d/function-units

@material/slider/_slider-theme.scss 77:5                                    @use
node_modules/@angular/material/slider/_slider-theme.scss 3:1                @use
node_modules/@angular/material/core/density/private/_all-density.scss 25:1  @forward
@angular/_index.scss 18:1                                                   @use
src/styles.scss 2:1                                                         root stylesheet
crisbeto commented 1 year ago

The deprecation warning will be fixed in the next patch release.

ghost commented 1 year ago

@crisbeto are you saying that the Migration Guide is not correct and that it should not be possible to set the size of the checkbox as it was before?

crisbeto commented 1 year ago

There were some mistakes in the migration guide. I've sent out https://github.com/angular/components/pull/26065 to fix them.

mduft commented 1 year ago

But.... setting density -1 as suggested in #26065 does not set the checkbox to the same size as it was before. Checkbox still blows up all my layouts...

crisbeto commented 1 year ago

What the guide is referring to is the touch target for the checkbox, not the checkbox itself. I think that the rectangle stays roughly the same size at all densities, but the margin around it becomes smaller.

mduft commented 1 year ago

That would maybe be worth mentioning, along with how to fix layouts as well xD

DemeSteve commented 1 year ago

But if I add this to the first line to my scss I get:

./src/styles.scss - Error: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Expected "(" or ".".
    ╷
157 │   $start: calc(icon - label);
    │                    ^
    ╵
  node_modules\@material\fab\_extended-fab-theme.scss 157:20                 @use
  @material\fab\_fab.scss 43:1                                               @use
  node_modules\@angular\material\button\_fab-theme.scss 2:1                  @use
  node_modules\@angular\material\core\density\private\_all-density.scss 4:1  @forward
  @angular\_index.scss 18:1                                                  @use
  src\styles.scss 1:1                                                        root stylesheet
JanneHarju commented 1 year ago

@DemeSteve what is this that you are referring to. I see only error message not anything that you tried. Did you give -2 or -2px. Because it only work with 0, -1, -2 and minimum as other have said. Density function then calculate correct values in it using given value.

DemeSteve commented 1 year ago

@JanneHarju Sorry. I've added this to my styles.scss file:

@use '@angular/material' as mat;
@include mat.checkbox-density(-1);

But I also get the same if I add only the first line.

Update: I was referencing the @material/fab component directly in my package.json, it was also an outdated version. After removing it worked.

wratho commented 1 year ago

the issue is the density change is losing to specificity due to being defined earlier in the process somehow?

you can fix it pretty easily with another selector around it. You can target it specifically if you want, but something as broad as body { @include mat.checkbox-density(-3); } appears to work just fine for us

wagnermaciel commented 1 year ago

Seems like this issue is resolved? If there are any specific pieces of documentation you'd like to see updated to make some of this more clear, please feel free to file a new issue

puneetv05 commented 1 year ago

I'm encountering the same issue with @include mat.checkbox-density(-1);. According to the migration documentation, this should alter the touch target size of the checkbox to match its previous dimensions. However, it doesn't seem to have the desired effect.

angular-automatic-lock-bot[bot] commented 1 year 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.