-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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(slider): vertical mode #1878
Conversation
blocked on #1794 |
0f87abb
to
39dae0c
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, just one minor comment.
@@ -106,21 +110,21 @@ describe('MdSlider', () => { | |||
}); | |||
|
|||
it('should update the track fill on click', () => { | |||
expect(trackFillElement.style.flexBasis).toBe('0%'); | |||
expect(trackFillElement.style.transform).toBe('scaleX(0)'); |
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 curious, why change from flex-basis
to transform
instead of changing the flex axis? Just the general preference to transform
?
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.
Yeah, I realized I could do it all with transform and figured that was preferable.
@@ -283,13 +360,20 @@ export class MdSlider implements ControlValueAccessor { | |||
this.value = this.min; | |||
break; | |||
case LEFT_ARROW: | |||
this._increment(-1); | |||
// It's kind of debatable what's the correct thing to do for inverted sliders. For a sighted |
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'd change "kind of debatable" bit and simply say that, for inverted sliders, we care more about the a11y experience than making having the keyboard shortcuts "look right" for sighted users. (I do think this is the right call).
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.
Done
92a38ce
to
3cb62bf
Compare
all the tests are probably broken T-T
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
had to rework the css a bit, but its for the better since all transitions are now either transform or opacity