-
Notifications
You must be signed in to change notification settings - Fork 2.1k
chore(demos): Add toggles for custom colors and disabled state for CSS-only Select #2095
Conversation
demos/select.html
Outdated
<div id="demo-css-only-select" class="mdc-select"> | ||
<select class="mdc-select__surface"> | ||
<option value="" selected>Pick a food group</option> | ||
<optgroup label="First Group"> |
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'm not sure we want to use <optgroup>
in our demos; the spec doesn't really have anything like it AFAIK.
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.
One question about <optgroup>
, otherwise LGTM
Codecov Report
@@ Coverage Diff @@
## master #2095 +/- ##
=======================================
Coverage 99.43% 99.43%
=======================================
Files 84 84
Lines 3720 3720
Branches 486 486
=======================================
Hits 3699 3699
Misses 21 21 Continue to review full report at Codecov.
|
selectCss.classList[cssAlternateColors.checked ? 'add' : 'remove']('demo-select-custom-colors'); | ||
}); | ||
cssDisabled.addEventListener('change', function() { | ||
cssDisabled.checked ? selectCss.setAttribute('disabled', 'disabled') : selectCss.removeAttribute('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.
Can't this be simplified to selectCss.disabled = cssDisabled.checked
?
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.
No, that doesn't work. The selectCss
variable is the div
container that contains the select and the bottom line. A div doesn't have a disabled value, so setting it requires that we add the attribute to the div tag itself.
Our css styles apply to a disabled selector (.mdc-select[disabled]
) to "Imitate native disabled functionality".
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.
Oh, oops, I'm dumb and was assuming it was the select element itself, and forgot the context.
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 to track the conversation here, I discussed this more offline with Will and realized that we've actually got some awkwardness with disabled CSS-only selects, in that you actually need to apply the disabled attribute to both the top-level element (to make it look disabled) and the select (to make it actually behave disabled). Needing to apply the disabled
attribute to an element for which it has no native functional meaning is really weird, and likely a remnant of select's structure prior to #1097.
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!
Add more demos for alternate colors and disabled state for css-only select.