-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2116 +/- ##
=======================================
Coverage 99.33% 99.33%
=======================================
Files 84 84
Lines 3769 3769
Branches 490 490
=======================================
Hits 3744 3744
Misses 25 25 Continue to review full report at Codecov.
|
@@ -200,37 +200,11 @@ To disable a list item, set `aria-disabled` to `"true"`, and set `tabindex` to ` | |||
</div> | |||
``` | |||
|
|||
### Using the Pure CSS Select |
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.
We should presumably also remove the tips/tricks section in this readme since it no longer applies (there is no longer a css-only MDC Select)
packages/mdc-select/README.md
Outdated
#### CSS Classes | ||
|
||
| Class | Description | | ||
| ------------------------ | ----------------------------------------------- | | ||
| `mdc-select` | A pure css `select` element | | ||
| `mdc-select` | Mandatory | |
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.
Include trailing period here for consistency with other entries
@@ -188,8 +188,7 @@ $mdc-select-menu-transition: transform 180ms $mdc-animation-standard-curve-timin | |||
} | |||
} | |||
|
|||
.mdc-select--disabled, |
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.
Just leaving a note here that the @each
after this will be removed in #2098 anyway, so it probably doesn't merit touching in this PR.
PS: This probably shouldn't just be a chore, since it directly affects users. |
Should it be a feature or fix? |
As weird as it is calling it a "feature", it is something that may affect users of the component so I'd call it that, and it probably warrants a breaking change note. (What'd we do when we removed CSS-only text field?) |
</div> | ||
</section> | ||
<div> | ||
<input type="checkbox" id="css-alternate-colors"> |
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.
You should also remove the JS that references these checkboxes that are no longer present.
Why remove the CSS only version of the form elements? When we were initially laying the foundation it was decided that these were necessary to allow for some level of support without JavaScript running and then allowing JS to enhance into full fidelity material design support. This allows for maximum performance and supporting clients that don't run JS to still have a usable experience. By removing the CSS only support, progressive enhancement with the project is dead. Which also can hurt mobile performance since users are left waiting for JavaScript to initialize the app code before they get a usable experience. |
I definitely share your concern, but at the same time, text field and select already had distinct DOM structures for CSS-only vs. JS components, which IMO already posed a problem for progressive enhancement. We recently noticed some problems pertaining to certain states (e.g. disabled state) of CSS-only text fields and selects. Moreover, we're going to be doing more style work and refactoring in this area, and feel that it makes sense to reduce styles and scope until the heavy lifting is done, then re-evaluate what the best approach looks like to accomplish this. |
I found some more things you can remove: This should be specific to actual material-components-web/packages/mdc-select/mdc-select.scss Lines 76 to 78 in 959398d
And for material-components-web/packages/mdc-select/mdc-select.scss Lines 88 to 89 in 959398d
|
Remove css only verion of select.