-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[Slider] Add keyboard support #3237
Conversation
Looks very cool @felipethome, I'll look at this implementation more in depth soon. 👍 |
Thanks @newoga , looking forward for your comments. |
} | ||
this._onDragStart(e); | ||
}, | ||
|
||
_onHandleKeyDown(e) { | ||
const pageDown = 34; |
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 the biggest fan of the current utils/key-code.js
utility, but maybe we could reuse that utility for now rather than declaring these constants for now since that utility is being used in other parts of the code base. Either way, these consts could probably be declared at the top of the file rather than in the key down handler.
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 would need to expand a little the key-code.js, but I think it is a nice idea use it, because the key codes are constants that a lot of components may want to use, so it makes sense to me have them as a separate module. Thanks for the tip!
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.
Don't touch these. I'm going to open an issue to discuss this further. #3348
@felipethome, Thanks in advance for deciding to break up your contributions in multiple PRs. It's going to make this a lot easier to review. Thanks for the demo too! 😄 I think this PR does a good job extending the functionality without increasing the API surface, so I'm really happy with that. I think there might be opportunities to improve how to define custom key event behavior to components without adding functions to the classes, though I'm not particularly hard pressed to address those problems in this PR. @oliviertassinari @alitaheri Could you take a look at this? What do you think? |
+1 on merging this in (when merge conflicts are resolved- and comments are addressed)! Not having onInput() support in the slider is a pretty big deal breaker for certain types of apps, and this PR fixes #1642. It does raise the point that onChange() in the material-ui slider would now behave like onInput() for regular type="range" html inputs (instead of the range input's onChange). I'm okay with that though, because at least it allows "instant" feedback from the slider- which an app I'm working on currently needs. |
@skratchdot nice point about the difference between |
@felipethome Do you have time to work on this (and more) 😄? The team is slammed getting some refactoring done for If you're busy that's fine, we can either ping you when we think we'll be ready for you to get the latest or we can try to incorporate your changes in a future PR. Let me know thanks again! |
@felipethome - good point about it already working like |
Hi @newoga , I don't mind at all. If there is no problem that for big tasks I do it during the weekends then I have time (small ones I can do during normal days). Thanks for the invitation 😄 |
Awesome, welcome! Almost everyone working on this is volunteer so you're not alone 😄 I know you have a lot of improvements lined up for this component and I don't want the other activity on this project to be a blocker, so I'm thinking it might be best to let you take ownership of the changes for Slider for now. Here are a couple things that are happening:
After we get step 1 done, we can also probably update this PR and I'll see if I can get another collaborator to provide feedback. Feel free to ping me on gitter if you have any questions. |
Ok, got it! Thanks |
This can continue now 😍 |
Hey @felipethome - no rush, but just checking if you're happy to continue this PR? |
Hi @mbrookes . I have! I had a personal problem these days, but I am ready to continue now |
@felipethome sorry to hear that. Please don't feel you need to worry about this if you have other things to attend to - you have done the hard work here already! If you would prefer, I can rebase on your behalf. |
Thank you so much! But I know you have a lot of things to do managing the project with the others. I can do it if you give me just a couple of days. |
Please take your time - it will still be here! :) |
Thanks @felipethome for being a team player and helping make this project great! As @mbrookes said, take care of yourself and take your time if you need it. We'll need you at your best for the longhaul 😄. At any time, let us know if there's anything we can do to make it easier for to stay involved (such as keeping PRs up to date) |
Thanks!!! That is why it is great to work in this project 😄 |
1065f9f
to
a9d0c04
Compare
I rebased and tested. It looks everything ok, but can you please give it a careful check? Because there were a lot of conflicts to solve. |
keycode(event) === 'page up' || | ||
keycode(event) === 'end' || | ||
keycode(event) === 'right' || | ||
keycode(event) === 'up'; |
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.
how about:
let action = false;
switch (keycode(event)) {
case 'page down':
case 'left':
case 'down':
action = 'decrease';
break;
case 'page up':
case 'right':
case 'up':
action = 'increase';
break;
case 'home':
action = 'home';
break;
case 'end':
action = 'end';
break;
}
if (action) {
// ...
switch(action) {
// ...
}
This will reduce 10 calls to keycode
to 1 and make the code a little more readable.
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.
Nice, I like it! can I just change let action = false
to let action = ''
? I like of having variables with the same type always
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.
Or simply let action;
should work too I think?
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.
yes it would. let action;
is a bit more expressive 👍
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.
Also true! (or in other words falsy 😄)
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.
😆 😆
Thanks a lot @felipethome for rebasing this, it's a must have feature 👍 |
Thank you too, I am incredibly glad of contributing. |
@felipethome Thanks so much for going through all the work. This is looking great 😄. I'll take another look when I'm more rested 😴 FYI - I opened #3436 based on our conversations earlier regarding this PR and started moving to callback refs in another PR while working on a separate issue. |
Also @felipethome, no pressure for this PR, but we're going to try to increase our test coverage for all of the components. @nathanmarks did an amazing job improving up testing setup recently and provided an example of unit testing in Avatar.spec.js. We also recently merged a test for @pradel offered to help add tests for our components, but it's likely going to have to be a group effort since our coverage is only currently 27.4% in master. If you have some time to work on developing a few tests for slider.jsx as you're improving it, that'd super helpful! |
Nice @newoga !! I am certain they will deprecate string refs soon. Tests will help so much, it will speed up the work a lot because every time we change something we need to worry if everything is ok. I will gladly do it! |
Awesome 🎉 This is the coverage report for slider based on this PR. It should help to identify testing areas: https://codecov.io/github/callemall/material-ui/src/slider.jsx?ref=63b3cfccf5e125afba77e2e0161463af8aa37893 |
Ok! Do you want the test for this PR or in another one? Is a problem if I start to work in the test on the weekend? |
@felipethome No problem! It was more of an FYI rather than an urgent request. Increasing the test coverage for this component is something we can strive for Thanks! |
I think this one is done now so I can start to work on the test. What do you think? Should I squash the commits? |
This looks good to me 😍 @callemall/material-ui Review, but don't merge before the alpha 👍 Thanks 👍 @felipethome The alpha will be released tomorrow, so this might take 1 or 2 to be merged. sorry 😅 |
Nice!! Thank you too 👍 👍 |
Felipe, please can you squash? |
[Slider] Use switch() to avoid multiple calls of keycode() [Slider] Fix lint errors
742318a
to
d63957d
Compare
Of course, just did it. |
[Slider] Add keyboard support
@felipethome Thanks! |
Great work @felipethome! Looking forward to what you have planned next for the component. 😄 Edit: If you're interested in working on anything else, let me know, there's lots of work to do. |
[Slider] Add keyboard support
The changes I want to propose are a little bit big, so I will divide it in two. I did a demo so you can check the final result.
I tested it against Chrome 47+, IE 10+, Firefox 43+, Safari 9.0, Android 4.4+
The changes I added in this PR:
preventDefault()
in the right place to avoid scroll and context menu when using touch events. Also I changed the approach of cancelling text selection using the *user-select
css property. I did that because, one, theautoPrefix.set()
will be removed and currently is not working which means text selection still happens, two, it makes possible to give focus to the handle when the user click directly on the track.Changes I want to add in the next one:
Obs: instead of following completely the specs for the label, I did a similar behaviour with the one found in the polymer paper slider as you can see in the demo.
Though I know these PR is big, I think there are valid ideas here. Hope you like.