-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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(range): add knobMoveStart and knobMoveEnd events #25011
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.
I am also indifferent to the event names. I do not think they are immediately recognizable as to what they are used for, but also cannot think of an alternative name that is more clear than ionChangeStart
and ionChangeEnd
.
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.
Looks good to me, great work on this 🎉
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.
Do we need to call ensureValueInBounds
when emitting the events?
ionic-framework/core/src/components/range/range.tsx
Lines 153 to 155 in a4803ec
value = this.ensureValueInBounds(value); | |
this.ionChange.emit({ value }); |
I don't know if the valueChanged
callback will update the value
prop with the clamped values.
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.
Great job! Don't forget to update the commit subject line to reflect the new event names.
Good catch, thanks! |
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build
) was run locally and any changes were pushednpm run lint
) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
The only way to listen for user interaction with an
ion-range
is theionChange
event. This doesn't fit all use cases, as it fires many times throughout a single drag gesture (every time the range's value updates). If a dev wants to listen for the user's gesture to finish, debouncing anionChange
listener wouldn't work either, as it would delay the drag-end handler by the debounce time.Issue URL: Resolves #17839
What is the new behavior?
Adds two new events:
ionKnobMoveStart
andionKnobMoveEnd
. These fire at, respectively, the start and end of a touch gesture or mouse drag on the range. Both events include the range'svalue
at the time. Keyboard interaction is also handled.Does this introduce a breaking change?
Other information