feat(slider): Implement continuous slider component #781
Conversation
traviskaufman
requested review from
amsheehan,
lynnmercier and
yeelan0319
and removed request for
lynnmercier
traviskaufman
force-pushed
the
feat/continuous-slider
branch
from
8449e9d to
9e35bd6
Compare
Codecov Report
@@ Coverage Diff @@
## master #781 +/- ##
==========================================
+ Coverage 99.92% 99.93% +<.01%
==========================================
Files 65 68 +3
Lines 2776 3119 +343
Branches 323 383 +60
==========================================
+ Hits 2774 3117 +343
Misses 2 2
Continue to review full report at Codecov.
|
demos/slider.html
Outdated
| <div class="mdc-toolbar__row"> | ||
| <section class="mdc-toolbar__section mdc-toolbar__section--align-start"> | ||
| <span class="catalog-back"> | ||
| <a href="/"><i class="material-icons"></i></a> |
There was a problem hiding this comment.
The new toolbar need a link to be wrapped around mdc-toolbar__icon--menu like the following:
<span class="catalog-back">
<a href="/" class="mdc-toolbar__icon--menu"><i class="material-icons"></i></a>
</span>
There was a problem hiding this comment.
Ah I see. I believe other demo code needs to be updated as well.
|
|
||
| ```js | ||
| // CommonJS | ||
| const {MDCSlider} = require('@material/slider/dist/mdc.slider'); |
There was a problem hiding this comment.
Correct me if I am wrong, but I think const MDCSlider = require('mdc-slider') works?
There was a problem hiding this comment.
It actually doesn't, since in package.json we specify the package name as @material/slider
|
Also there is a noticeable flash in thumb when user set value of slider to 0 (or they provide no value), which I don't feel it is a good UX experience. The reason causing that is because we always add I think there are potentially 2 strategies here.
IMHO, I prefer we handle the logic if that is not too much of overhead. WDYT? |
| }); | ||
|
|
||
| slider.listen('MDCSlider:change', function() { | ||
| committedValue.textContent = slider.value; |
There was a problem hiding this comment.
nit: would demonstrate that the event come with mdc-slider instance be more informative?
There was a problem hiding this comment.
Sorry but I'm not sure what you mean here...
| assert.equal(foundation.getStep(), 0); | ||
| }); | ||
|
|
||
| test('#constructor sets the disabled state to enabled', () => { |
There was a problem hiding this comment.
super nit: the default disabled state
| foundation.layout(); | ||
| foundation.layout(); | ||
|
|
||
| assert.equal(numComputations, 2); |
There was a problem hiding this comment.
Maybe use td.verify(mockAdapter.computeBoundingRect(), {times: 2}); and remove the counter instead?
There was a problem hiding this comment.
If you try and td.verify a method that's been mocked out, testdouble will emit a warning (rightfully so). Added an explanation as to why we need to stub and verify , thus use this method, in a comment.
There was a problem hiding this comment.
Works for me. I didn't noticed that warning message, good testdouble!
| raf.restore(); | ||
| }); | ||
|
|
||
| test('#getMax/#setMax basic functionality', () => { |
There was a problem hiding this comment.
super nit: feel the test description should be more descriptive.
How about "Slider Max getter and setter work correctly"?
There was a problem hiding this comment.
I think "correctly" is equally as vague, if not more. What is "correctly"? How many aspects of the functionality does "correctly" cover? How about something like #setMax/#getMax sets/retrieves the maximum value, respectively. It's more verbose but also more descriptive, plus it's precise and accurate as to what it's testing.
| raf.restore(); | ||
| }); | ||
|
|
||
| test('#getMin/#setMin basic functionality', () => { |
There was a problem hiding this comment.
Super nit: same thing here.
| raf.restore(); | ||
| }); | ||
|
|
||
| test('#getStep/#setStep basic functionality', () => { |
| raf.restore(); | ||
| }); | ||
|
|
||
| test('#isDisabled/#setDisabled basic functionality', () => { |
| </a> | ||
| </div>--> | ||
|
|
||
| > **Status**: |
There was a problem hiding this comment.
This is a very novel way to document the development state of components that have multiple permutations.
packages/mdc-slider/README.md
Outdated
| > - [ ] Discrete Sliders | ||
|
|
||
| MDC Slider provides an implementation of the Material Design slider component. It is modeled after | ||
| the browser's `<input type="range">` element. Sliders are fully RTL-aware, and conforms to the |
There was a problem hiding this comment.
"...fully RTL-aware, and conform to the..."
packages/mdc-slider/README.md
Outdated
| Note that **vertical sliders and range (multi-thumb) sliders are not supported, due to their absence | ||
| from the material design spec**. | ||
|
|
||
| Also note that we have taken certain deviations from the UX within the spec, so there may be some |
There was a problem hiding this comment.
Perhaps we can be more concrete in our language here. What specifically changed? What should I expect?
demos/slider.html
Outdated
| <section class="hero"> | ||
| <div id="hero-slider-wrapper"> | ||
| <div id="hero-slider" class="mdc-slider" tabindex="0" | ||
| role="slider" aria-valuemin="0" aria-valuemax="100" aria-valuenow="0" |
There was a problem hiding this comment.
It may be an improvement to set aria-valuenow to something ~30. I think it speaks more clearly as to what the track and thumb are used for if we have some non-zero value set on page load. Just my opinion.
| } | ||
|
|
||
| .mdc-theme--dark { | ||
| background-color: #333; |
There was a problem hiding this comment.
I noticed that we handle the contrast of component/dark background by applying the accent color to the component. I feel this would be useful in other demos as well (mdc-switch comes to mind).
There was a problem hiding this comment.
Agreed and noted. Should I file an issue? Should we do this within demos.scss and make a general note somewhere?
There was a problem hiding this comment.
I think an issue is appropriate. That way we can toss up a help-wanted label and see if anyone bites.
| deregisterThumbContainerInteractionHandler: (type, handler) => { | ||
| this.thumbContainer_.removeEventListener(type, handler); | ||
| }, | ||
| registerBodyInteractionHandler: (type, handler) => { |
There was a problem hiding this comment.
I feel we are using document.addEventListener(type, handler) in a few components. We should be attaching them to the body because
- If we want to check any attributes/classnames/etc... of an EventTarget, it doesn't make sense to develop against the
documentsince it doesn't share any properties with an HTMLElement. - For the same reason as above, testing aforementioned checks via
domEvents.emit(document, 'click')is impossible. e.g.:
// something that gets called in response to _either_ an element
// click or a document click event
function eventHandler(evt) {
if (someAdapterMethodThatChecksForAnAttr(evt.target)) {
doSomething();
}
}
function eventHandlerTest() {
...
domEvents.emit(document, 'click');
domEvents.emit(someElement, 'click');
...
// Results in event targets with completely different properties.
// This makes it very kludgy to check for event target properties in
// our foundations/components
}Also, a person browsing a website can't technically click on the document.
There was a problem hiding this comment.
👍 techdebt issue?
packages/mdc-slider/mdc-slider.scss
Outdated
| } | ||
|
|
||
| .mdc-slider--active { | ||
| &:not(.mdc-slider--disabled) .mdc-slider__thumb { |
There was a problem hiding this comment.
yeah dunno why I did that...removing :not(.mdc-slider--disabled) since the slider can't ever be active if also disabled
|
|
||
| suite('MDCSliderFoundation - keyboard events'); | ||
|
|
||
| function createFakeEvent(info = {}) { |
There was a problem hiding this comment.
This would be useful in a lot of places. Consider pulling it out into one of the helper files?
There was a problem hiding this comment.
Are there places you can identify currently where this would be useful? Otherwise I'm skeptical hesitant to prematurely refactor it.
| raf.restore(); | ||
| }); | ||
|
|
||
| test('on any other keydown does nothing', () => { |
There was a problem hiding this comment.
nit: I mean... kind of. This really just tests if any other key is the Enter key.
| this.adapter_.deregisterResizeHandler(this.resizeHandler_); | ||
| } | ||
|
|
||
| layout() { |
There was a problem hiding this comment.
This updates UI, but does nothing to alert the user of the value change. e.g.: If a user programmatically changes the step value, we see the slider move to the closest step. However, we don't get an event for the value change.
There was a problem hiding this comment.
I think that's fine. When you update an HTMLInputElement's value prop programmatically you don't get an event either, so this keeps our behavior consistent with the way the DOM API behaves.
I'm not sure we can handle this overhead automatically. Because the slider programmatically sets all of its values, we have to wait for the script to execute for those styles to automatically be applied, thus there's always a potential for FOUC. I think your suggestion regarding manually adding the class for the off state is a good one; I'll go ahead and add that to the README. |
- Add continuous slider component - Check in `package-lock.json`, part of npm5/node8 - Disable stylelint for `demos.css` Part of #25
traviskaufman
force-pushed
the
feat/continuous-slider
branch
from
9e35bd6 to
0fc9ca9
Compare
|
All comments addressed. PTAL! |
|
Agree to your concern about the racing condition in initialization, also noticed you changed the initial value of the demo. 👍 PR looks all good to me. |
|
Spoke with @amsheehan offline who said LGTM. |
package-lock.json, part of npm5/node8demos.cssPart of #25