Skip to content

fix(#2659): useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End #2661

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Feb 17, 2022
Merged

fix(#2659): useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End #2661

merged 19 commits into from
Feb 17, 2022

Conversation

majornista
Copy link
Collaborator

@majornista majornista commented Dec 8, 2021 •
edited
Loading

Closes #2659

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue 2659.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. Open Slider Hooks Single Story: https://reactspectrum.blob.core.windows.net/reactspectrum/92cb6eea1c50f026636dd7e3ae9f8cb7c57ab6d5/storybook/index.html?path=/story/slider-hooks--single
  2. Navigate to focus the Slider example.
  3. Press ArrowRight to increment the value by 1. Notice in Actions panel, both onChange and onChangeEnd fire.
  4. Press ArrowLeft to decrement the value by 1. Notice in Actions panel, both onChange and onChangeEnd fire.
  5. With slider at the minimum value, 0, press ArrowLeft to decrement the value by 1. Notice in Actions panel, only onChangeEnd fires, because the value has not changed.
  6. Press End key (fn+ArrowRight on a macOS laptop), to jump to the maximum value. Notice in Actions panel, both onChange fires and onChangeEnd fire.
  7. With slider at the maximum value, 100, press ArrowRight to increment the value by 1. Notice in Actions panel, only onChangeEnd fires, because the value has not changed.
  8. Press Home key (fn+ArrowLeft on a macOS laptop), to jump to the minimum value. Notice in Actions panel, both onChange and onChangeEnd fire.
  9. Press PageUp key (fn+ArrowUp on a macOS laptop), to increment by 10. Notice in Actions panel, both onChange and onChangeEnd fire.
  10. Press PageUp key (fn+ArrowDown on a macOS laptop), to decrement by 10. Notice in Actions panel, both onChange and onChangeEnd fire.
  11. Increment or decrement by step using the arrow keys. Notice in Actions panel, both onChange and onChangeEnd fire for each arrow key press.

🧢 Your Project:

Adobe/Accessibility

@majornista majornista requested review from LFDanLu and dkario December 8, 2021 21:25
@adobe-bot
Copy link

Build successful! 🎉

@majornista majornista changed the title useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End fix(#2659): useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End Dec 8, 2021
This was referenced Dec 8, 2021
Merged
Closed
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

LFDanLu
LFDanLu previously approved these changes Jan 26, 2022
View reviewed changes
Copy link
Member

@LFDanLu LFDanLu left a 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, tested locally and verified the behavior with the slider hooks story. For those who noticed that the Slider stories don't have a onChangeEnd that fires, that is a story issue, it's missing a onChangeEnd storybook action

@adobe-bot
Copy link

Build successful! 🎉

@majornista majornista requested a review from LFDanLu January 27, 2022 14:59
@majornista
Copy link
Collaborator Author

Looks good to me, tested locally and verified the behavior with the slider hooks story. For those who noticed that the Slider stories don't have a onChangeEnd that fires, that is a story issue, it's missing a onChangeEnd storybook action

@LFDanLu See 69630ce.

LFDanLu
LFDanLu previously approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

reidbarber
reidbarber previously approved these changes Feb 1, 2022
View reviewed changes
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested Slider, RangeSlider, and Slider (hooks) stories.

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

snowystinger
snowystinger previously approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

LFDanLu
LFDanLu previously approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, verified that onChangeEnd fires now even if the values haven't change (aka interactions at either extremes of the slider) Other interactions seem to work as expected still and pageSize is properly respected when set

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@snowystinger snowystinger dismissed stale reviews from LFDanLu and themself via f9e6854 February 17, 2022 19:29
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

LFDanLu
LFDanLu approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, slider step/page step still works as expected, same with PgUp/Down/Home/End

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@snowystinger snowystinger merged commit 1aa66e6 into main Feb 17, 2022
@snowystinger snowystinger deleted the Issue-2659-useSliderThumb-onChangeEnd branch February 17, 2022 23:29
majornista added a commit to majornista/react-spectrum-v3 that referenced this pull request Feb 18, 2022
@majornista @LFDanLu @snowystinger
fix(adobe#2659): useSliderThumb: fire onChangeEnd for PageUp/PageDown…
f2be751 
…/Home/End (adobe#2661)

* useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End

* fix(adobe#2659): add onChangeEnd event handler to Slider/RangeSlider stories

Per adobe#2661 (review)

* Have slider work like color area (adobe#2819)

* Match Slider implementation to color area

* fix lint

* make step optional with a default

* add pagesize

* Use native slider pagesize calculation

* @trivial remove irrelevant comment and use the declared variable for direction === 'rtl'

Co-authored-by: Michael Jordan <[email protected]>

* fix(adobe#2659): add pageSize stories

* simplify slider move logic

* a little more cleanup

Co-authored-by: Daniel Lu <[email protected]>
Co-authored-by: Robert Snow <[email protected]>
Co-authored-by: Daniel Lu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devongovett devongovett devongovett approved these changes

@snowystinger snowystinger snowystinger approved these changes

@LFDanLu LFDanLu LFDanLu approved these changes

@reidbarber reidbarber reidbarber left review comments

@dannify dannify Awaiting requested review from dannify

@dkario dkario Awaiting requested review from dkario

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

useSliderThumb doesn't fire onChangeEnd on Home/End/PageUp/PageDown key
7 participants
@majornista @adobe-bot @devongovett @snowystinger @reidbarber @LFDanLu @dannify