-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[TextField: Spinner] Support press and hold to increment/decrement value #573
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
[TextField: Spinner] Support press and hold to increment/decrement value #573
Conversation
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.
@tanhengyeow thanks so much for the contribution! On first glance, this is looking like a good step in the right direction. Can you add some tests to this change to cover all new code paths, and in particular all new props added to Spinner
?
|
||
@autobind | ||
private handleButtonPress(onChange: Function) { | ||
this.buttonPressTimer = setInterval(onChange, 200); |
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.
200
might be a bit long and I think we should consider adding a setTimeout
first. That way when clicking and holding, there is an initial delay of maybe 500
then the number quickly increments maybe at an interval of 100
. Thoughts?
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.
this.buttonPressTimer = setInterval(onChange, 200); | |
let interval = 200; | |
const onChangeInterval = () => { | |
if (interval > 50) { | |
interval -= 10; | |
} else { | |
interval = 50; | |
} | |
onChange(); | |
this.buttonPressTimer = window.setTimeout(onChangeInterval, interval); | |
}; | |
this.buttonPressTimer = window.setTimeout(onChangeInterval, interval); |
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.
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 like this suggestion of gradually increasing the speed. Overall, this looks great!
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.
Overall, looks good! As @danrosenthal mentioned, this could use tests though.
Hi @tanhengyeow! Are you able to provide any updates around tests? Thanks! |
Hey everyone, thanks for the review 😄 The suggestion sounds great and I'll look into adding tests :) I'm having exams now so I can only get back to this during December, hope that's fine :D |
Totally fine. I'll add |
82d4eae
to
d2653e7
Compare
All feedback has been addressed
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 went ahead a tied this one up
I might be missing something here, but when I tophat, it still just seems to increment and decrement by one digit. |
When you |
bcecf24
to
2215967
Compare
🎉 Thanks for your contribution to Polaris React! |
WHY are these changes introduced?
Resolves #420
WHAT is this pull request doing?
Support press and hold to increment/decrement value in
Spinner
of the numericTextField
.Copy-paste this code in
playground/Playground.tsx
: