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.

Sign up for GitHub

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

Jump to bottom

fix(shape): Fix errors related to multi-value shape categories #4547

Merged
merged 13 commits into from
Apr 2, 2019
Merged

fix(shape): Fix errors related to multi-value shape categories #4547

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bec54cf
fix(notched-outline): Warn when radius resolves to multiple values
Mar 27, 2019
b8f8538
WIP Missed one variable rename
Mar 28, 2019
95ede35
Merge branch 'master' into fix/textfield-select-shape-warn
kfranqueiro Mar 28, 2019
6307eba
WIP Fixes to correctly support categories specifying multiple radii
Mar 29, 2019
937f539
WIP Shorten warning message
Mar 29, 2019
5904bd1
WIP Fix lint
Mar 29, 2019
dfe2347
WIP Add clarifying comment
Mar 29, 2019
f76d287
WIP Add examples
Apr 2, 2019
726b27a
fix(shape): Added new test to check radius list override
abhiomkar Apr 2, 2019
d05f6ae
Merge remote-tracking branch 'origin/master' into fix/textfield-selec…
abhiomkar Apr 2, 2019
242c352
fix(shape): Fix lint errors
abhiomkar Apr 2, 2019
875b8e3
fix(shape): Fix specificity in tests and init components
abhiomkar Apr 2, 2019
308b6e7
fix(shape): Updated golden.json
abhiomkar Apr 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/mdc-notched-outline/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@
@mixin mdc-notched-outline-shape-radius($radius, $rtl-reflexive: false) {
$radius: mdc-shape-prop-value($radius);

@if (length($radius) > 1) {
// stylelint-disable-next-line max-line-length
@warn "mdc-notched-outline-shape-radius only supports a single radius; see https://github.com/material-components/material-components-web/issues/4140";
}

$radius: nth($radius, 1);

.mdc-notched-outline__leading {
@include mdc-shape-radius(mdc-shape-mask-radius($radius, 1 0 0 1), $rtl-reflexive: true);

Expand Down
15 changes: 9 additions & 6 deletions packages/mdc-select/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,12 @@
}

@mixin mdc-select-outline-shape-radius($radius, $rtl-reflexive: false) {
$resolved-radius: mdc-shape-resolve-percentage-radius($mdc-select-height, $radius);
$resolved-radius: nth(mdc-shape-resolve-percentage-radius($mdc-select-height, mdc-shape-prop-value($radius)), 1);

@if (length(mdc-shape-prop-value($radius)) > 1) {
// stylelint-disable-next-line max-line-length
@warn "mdc-select-outline-shape-radius only supports a single radius; see https://github.com/material-components/material-components-web/issues/4140";
}

.mdc-notched-outline {
@include mdc-notched-outline-shape-radius($resolved-radius, $rtl-reflexive);
Expand All @@ -118,21 +123,19 @@
@include mdc-shape-radius($resolved-radius, $rtl-reflexive);
}

$radius-pixels: mdc-shape-prop-value($resolved-radius);

@if ($radius-pixels > $mdc-notched-outline-leading-width) {
@if ($resolved-radius > $mdc-notched-outline-leading-width) {
.mdc-select__native-control {
@include mdc-rtl-reflexive-property(
padding,
$radius-pixels + $mdc-notched-outline-padding,
$resolved-radius + $mdc-notched-outline-padding,
$mdc-select-arrow-padding
);
}

+ .mdc-select-helper-text {
@include mdc-rtl-reflexive-property(
margin,
$radius-pixels + $mdc-notched-outline-padding,
$resolved-radius + $mdc-notched-outline-padding,
$mdc-select-outline-label-offset
);
}
Expand Down
103 changes: 52 additions & 51 deletions packages/mdc-shape/_functions.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@
// mdc-shape-resolve-percentage-radius(36px, 50%) => `18px` (i.e., 36px / 2)
//
@function mdc-shape-resolve-percentage-radius($component-height, $radius) {
$radius: mdc-shape-prop-value($radius);

@if type-of($radius) == "list" {
$radius-value: ();

Expand All @@ -70,39 +72,10 @@
}
}

@function mdc-shape-resolve-percentage-for-corner_($component-height, $radius) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this function lower in the file verbatim, to group internal functions together.

$radius-value: mdc-shape-prop-corner-value_($radius);

@if type-of($radius-value) == "number" and unit($radius-value) == "%" {
// Converts the percentage to number without unit. Example: 50% => 50.
$percentage: $radius-value / ($radius-value * 0 + 1);

@return $component-height * ($percentage / 100);
} @else {
@return $radius-value;
}
}

//
// Strips unit from number. This is accomplished by dividing the value by itself to cancel out units, while resulting
// in a denominator of 1.
//
// Examples:
//
// 50% => 50
//
@function mdc-shape-strip-unit_($number) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was apparently not being used whatsoever, so I removed it.

@if type-of($number) == "number" and not unitless($number) {
@return $number / ($number * 0 + 1);
}

@return $number;
}

//
// Returns $radius value of shape category - `large`, `medium` or `small`.
// Otherwise, it returns the $radius itself if valid.
// $radius can be a single value or list of up to 4.
// $radius can be a single value, or a list of up to 4 values.
kfranqueiro marked this conversation as resolved.
Show resolved Hide resolved
//
// Examples:
//
Expand All @@ -114,15 +87,27 @@
@error "Invalid radius: '#{$radius}' is more than 4 values";
}

$radius-value: ();
$radius-values: ();

@each $corner in $radius {
$radius-value: append($radius-value, mdc-shape-prop-corner-value_($corner));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up merging the logic of mdc-shape-prop-corner-value_ into this function after some of the finagling I did led to this function otherwise pretty much not doing anything important in itself besides validating list length.

@for $i from 1 through length($radius) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why is this changed from @each to @for?

Copy link
Contributor Author

@kfranqueiro kfranqueiro Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need the index to correlate between the given radius list and the fully-expanded list of radii for any categories that are referenced.

$corner: nth($radius, $i);

@if map-has-key($mdc-shape-category-values, $corner) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if small value is 4px 0 4px 0 and I apply this mixin with this value - mdc-button-shape-radius(small 0 small 0)?

I'm OK not supporting something like mdc-button-shape-radius(small 0 small 0) if that makes the implementation complex.

Copy link
Contributor Author

@kfranqueiro kfranqueiro Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would result in 4px 0 4px 0. It maps each respective corner from the shape category. This also allows existing cases that expand to e.g. small small 0 0 to continue to work as-is, even if small is only a single value (it'll expand to the same value on all 4 corners).

// If a category is encountered within a list of radii, apply the category's value for the corresponding corner
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior maximizes compatibility with the existing behavior which clearly expected shape categories to be applicable to individual corners (e.g. small small 0 0). This will effectively behave identically to the existing behavior in cases where a shape category is a single radius, while making list-of-radii category values actually functional.

$radius-values:
append($radius-values, nth(mdc-shape-unpack-radius_(map-get($mdc-shape-category-values, $corner)), $i));
} @else {
$radius-values: append($radius-values, mdc-shape-validate-radius-value_($corner));
}
}

@return $radius-value;
@return $radius-values;
} @else {
@return mdc-shape-prop-corner-value_($radius);
@if map-has-key($mdc-shape-category-values, $radius) {
@return map-get($mdc-shape-category-values, $radius);
} @else {
@return mdc-shape-validate-radius-value_($radius);
}
}
}

Expand All @@ -147,34 +132,50 @@
@error "Expected masked-corners of length 4 but got '#{length($masked-corners)}'.";
}

@if length($radius) == 3 {
$radius: nth($radius, 1) nth($radius, 2) nth($radius, 3) nth($radius, 2);
} @else if length($radius) == 2 {
$radius: nth($radius, 1) nth($radius, 2) nth($radius, 1) nth($radius, 2);
} @else if length($radius) == 1 {
$radius: $radius $radius $radius $radius;
}
$radius: mdc-shape-unpack-radius_($radius);

@return if(nth($masked-corners, 1) == 1, nth($radius, 1), 0)
if(nth($masked-corners, 2) == 1, nth($radius, 2), 0)
if(nth($masked-corners, 3) == 1, nth($radius, 3), 0)
if(nth($masked-corners, 4) == 1, nth($radius, 4), 0);
}

@function mdc-shape-prop-corner-value_($radius) {
@if map-has-key($mdc-shape-category-values, $radius) {
@return map-get($mdc-shape-category-values, $radius);
} @else if mdc-shape-is-valid-radius-value_($radius) {
//
// Unpacks shorthand values for border-radius (i.e. lists of 1-3 values).
// If a list of 4 values is given, it is returned as-is.
// TODO: This is private for purposes of getting it into a patch; make it public for a future minor/major release.
kfranqueiro marked this conversation as resolved.
Show resolved Hide resolved
//
@function mdc-shape-unpack-radius_($radius) {
@if length($radius) == 4 {
@return $radius;
} @else {
@error "Invalid radius: '#{$radius}' radius is not supported";
} @else if length($radius) == 3 {
@return nth($radius, 1) nth($radius, 2) nth($radius, 3) nth($radius, 2);
} @else if length($radius) == 2 {
@return nth($radius, 1) nth($radius, 2) nth($radius, 1) nth($radius, 2);
} @else if length($radius) == 1 {
@return $radius $radius $radius $radius;
}

@return map-get($mdc-shape-category-values, $radius);
@error "Invalid radius: '#{$radius}' is more than 4 values";
}

@function mdc-shape-is-valid-radius-value_($radius) {
@function mdc-shape-resolve-percentage-for-corner_($component-height, $radius) {
@if type-of($radius) == "number" and unit($radius) == "%" {
// Converts the percentage to number without unit. Example: 50% => 50.
$percentage: $radius / ($radius * 0 + 1);

@return $component-height * ($percentage / 100);
} @else {
@return $radius;
}
}

@function mdc-shape-validate-radius-value_($radius) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has been rewritten to emit an error, akin to the verify functions in feature-targeting, so that there's less code to write in places that call this.

$is-number: type-of($radius) == "number";

@return $is-number or str_index($radius, "var(") or str_index($radius, "calc(");
@if not ($is-number or str_index($radius, "var(") or str_index($radius, "calc(")) {
@error "Invalid radius: #{$radius}";
}

@return $radius;
}
15 changes: 9 additions & 6 deletions packages/mdc-textfield/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,21 +217,24 @@
}

@mixin mdc-text-field-outline-shape-radius($radius, $rtl-reflexive: false) {
$resolved-radius: mdc-shape-resolve-percentage-radius($mdc-text-field-height, $radius);
$resolved-radius: nth(mdc-shape-resolve-percentage-radius($mdc-text-field-height, mdc-shape-prop-value($radius)), 1);

@if (length(mdc-shape-prop-value($radius)) > 1) {
// stylelint-disable-next-line max-line-length
@warn "mdc-text-field-outline-shape-radius only supports a single radius; see https://github.com/material-components/material-components-web/issues/4140";
}

.mdc-notched-outline {
@include mdc-notched-outline-shape-radius($resolved-radius, $rtl-reflexive);
}

$radius-pixels: mdc-shape-prop-value($resolved-radius);

@if ($radius-pixels > $mdc-notched-outline-leading-width) {
@if ($resolved-radius > $mdc-notched-outline-leading-width) {
.mdc-text-field__input {
@include mdc-rtl-reflexive-property(padding, $radius-pixels + $mdc-notched-outline-padding, 0);
@include mdc-rtl-reflexive-property(padding, $resolved-radius + $mdc-notched-outline-padding, 0);
}

+ .mdc-text-field-helper-line {
@include mdc-rtl-reflexive-property(padding, $radius-pixels + $mdc-notched-outline-padding, $radius-pixels);
@include mdc-rtl-reflexive-property(padding, $resolved-radius + $mdc-notched-outline-padding, $resolved-radius);
}
}
}
Expand Down