-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2674 +/- ##
=========================================
Coverage ? 98.25%
=========================================
Files ? 98
Lines ? 4231
Branches ? 540
=========================================
Hits ? 4157
Misses ? 74
Partials ? 0
Continue to review full report at Codecov.
|
packages/mdc-select/_mixins.scss
Outdated
} | ||
|
||
@mixin mdc-select-hover-outline-color_($color) { | ||
&:not(.mdc-select__native-control:focus) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be moved inline:
&:not(.mdc-select__native-control:focus) .mdc-select__native-control:hover ~ { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this ?
&:not(.mdc-select__native-control:focus) .mdc-select__native-control:hover ~ {
@include mdc-notched-outline-idle-color($color);
}
&:not(.mdc-select__native-control:focus) .mdc-notched-outline {
@include mdc-notched-outline-color($color);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it is like this:
&:not(.mdc-select__native-control:focus) .mdc-select__native-control:hover ~ {
@include mdc-notched-outline-idle-color($color);
.mdc-notched-outline {
@include mdc-notched-outline-color($color);
}
}
|
||
if (openNotch) { | ||
const labelScale = numbers.LABEL_SCALE; | ||
const labelWidth = this.adapter_.getLabelWidth() * labelScale; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want a dense alternative here. See https://github.com/material-components/material-components-web/blob/feat/select/add-outlined-variant/packages/mdc-textfield/foundation.js#L205
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The select doesn't have a dense
variant yet.
packages/mdc-select/index.js
Outdated
return { | ||
notchOutline: (labelWidth, isRtl) => { | ||
if (this.outline_) { | ||
return this.outline_.notch(labelWidth, isRtl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to return anything.
packages/mdc-select/index.js
Outdated
}, | ||
closeOutline: () => { | ||
if (this.outline_) { | ||
return this.outline_.closeNotch(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
packages/mdc-select/index.js
Outdated
return { | ||
floatLabel: (shouldFloat) => { | ||
if (this.label_) { | ||
return this.label_.float(shouldFloat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nor in this?
.mdc-select__native-control { | ||
@include mdc-rtl-reflexive-property(padding, $mdc-select-label-padding, $mdc-select-arrow-padding); | ||
|
||
display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea...this looks exactly like the @mixin mdc-text-field-outlined_
piece of sass. What if we put this as a mixin in the notched-outline sass?
packages/mdc-select/constants.js
Outdated
/** @enum {number} */ | ||
const numbers = { | ||
LABEL_SCALE: 0.75, | ||
DENSE_LABEL_SCALE: 0.923, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove since we don't have dense
packages/mdc-select/_mixins.scss
Outdated
@@ -100,3 +126,41 @@ | |||
@mixin mdc-select-dd-arrow-svg-bg_($fill-hex-number: 000000, $opacity: .54) { | |||
background-image: url(data:image/svg+xml,%3Csvg%20width%3D%2210px%22%20height%3D%225px%22%20viewBox%3D%227%2010%2010%205%22%20version%3D%221.1%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20xmlns%3Axlink%3D%22http%3A%2F%2Fwww.w3.org%2F1999%2Fxlink%22%3E%0A%20%20%20%20%3Cpolygon%20id%3D%22Shape%22%20stroke%3D%22none%22%20fill%3D%22%23#{$fill-hex-number}%22%20fill-rule%3D%22evenodd%22%20opacity%3D%22#{$opacity}%22%20points%3D%227%2010%2012%2015%2017%2010%22%3E%3C%2Fpolygon%3E%0A%3C%2Fsvg%3E); | |||
} | |||
|
|||
@mixin mdc-select-outline-corner-radius_($radius) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be public
packages/mdc-select/_variables.scss
Outdated
@@ -18,6 +18,7 @@ | |||
|
|||
$mdc-select-arrow-padding: 26px; | |||
$mdc-select-label-padding: 16px; | |||
$mdc-select-border-radius: 4px !default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this !default in favor of the sass mixin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to open a bug since the outline radius mixin is set as public, but is private
45b291a
to
7a92d56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes: #2176