select(refactor): decouple label from select #2244
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2244 +/- ##
==========================================
- Coverage 99.13% 98.72% -0.41%
==========================================
Files 90 93 +3
Lines 3830 3857 +27
Branches 496 501 +5
==========================================
+ Hits 3797 3808 +11
- Misses 33 49 +16
Continue to review full report at Codecov.
|
| assert.isTrue(label.classList.contains('foo')); | ||
| }); | ||
|
|
||
| test('adapter#removeClassFromLabel removes a class from the label', () => { |
There was a problem hiding this comment.
this test will be covered in #2244 when MDCSelect uses MDCFloatingLabel
packages/mdc-select/README.md
Outdated
| @@ -280,6 +280,26 @@ The `menuFactory` function is passed an `HTMLElement` and is expected to return | |||
| instance attached to that element. This is mostly used for testing purposes, but it's there if you | |||
| need it nonetheless. | |||
|
|
|||
| #### Instantiating using a custom `MDCSelectLabel` component. | |||
There was a problem hiding this comment.
I don't think we should be adding docs for instantiating this sub component.
packages/mdc-select/foundation.js
Outdated
| @@ -153,9 +152,9 @@ export default class MDCSelectFoundation extends MDCFoundation { | |||
| if (this.selectedIndex_ >= 0) { | |||
| selectedTextContent = this.adapter_.getTextForOptionAtIndex(this.selectedIndex_).trim(); | |||
| this.adapter_.setAttrForOptionAtIndex(this.selectedIndex_, 'aria-selected', 'true'); | |||
| this.adapter_.addClassToLabel(cssClasses.LABEL_FLOAT_ABOVE); | |||
| this.adapter_.floatLabel(true); | |||
There was a problem hiding this comment.
We looked up the spec yesterday and saw that this should be moved to the open function, and line 157 should be moved to the closed function.
packages/mdc-select/mdc-select.scss
Outdated
| @import "@material/animation/functions"; | ||
| @import "@material/animation/variables"; | ||
| // @import "@material/animation/variables"; |
There was a problem hiding this comment.
Probably don't need the commented out import.
|
|
||
| @import "@material/theme/mixins"; | ||
|
|
||
| @mixin mdc-select-floating-label-color($color, $opacity: 1) { |
There was a problem hiding this comment.
Should this be private ?
There was a problem hiding this comment.
In the current implementation in #2237 it is public. It needs to be public for other modules to change the color. Why do you think it needs to be private?
There was a problem hiding this comment.
I thought public color mixins were supposed to explicitly prevent styling the disabled state
:not(.mdc-text-field--disabled).
That doesn't really make sense for the separate package though, so maybe it doesn't matter.
There was a problem hiding this comment.
I think it doesn't make sense to have that in the public mixin, but I guess the question then is, should we allow the end developer to style the color in the disabled state?
Also in the current state being in the label module, we shouldn't add in parent classes like .mdc-text-field--disabled or .mdc-select--disabled
There was a problem hiding this comment.
I think we're explicitly preventing them from styling the disabled state. It doesn't make sense to include the parent class, we added proxy mixins from the parent to the line-ripple module to prevent customizing the disabled state.
|
Also RE: color when menu opened and --float-above, is not currently the primary color in 0.30.0. I think this should be a separate issue as that feature has never been in the code base. |
related #1982