-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
feat(select): Add outlined variant #2674
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