-
Notifications
You must be signed in to change notification settings - Fork 345
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
Multi-Thumb Slider: Add ability to move thumbs by clicking rail #3172
Open
jongund
wants to merge
5
commits into
main
Choose a base branch
from
update/multi-thumb-slider
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+61
−25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcking65
changed the title
Updated multi-thumb slider to support rail click
Multi-Thumb Slider: Add ability to move thumbs by clicking rail
Nov 12, 2024
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: PR 3172 - Add rail click functionality to multi-thumb slider<jugglinmike> github: https://github.com//pull/3172 <jugglinmike> Matt_King: This adds the functionality to the multi-thumb slider which we discussed a few weeks ago. It allows people to click on the rail to move the sliders--it moves the closest one <jugglinmike> Matt_King: This changed JavaScript and CSS <jugglinmike> Matt_King: It seems there's a failing check... It says the JavaScript linter is failing, jongund <jugglinmike> Matt_King: Does anybody think we need to change any of the documentation about the example? It's a change to mouse behavior--it doesn't change keyboard. It doesn't change roles, states or properties, either. <jugglinmike> jongund: The CSS change allows pointer events to travel through a particular element <jugglinmike> Matt_King: So this doesn't change the visual appearance? <jugglinmike> jongund: No. However, it occurs to me that some folks might prefer the rail itself to be a few pixels taller now that it is click-able <jugglinmike> Matt_King: There's no change to design, so we need somebody to test the functionality, and we need someone to perform a code review <jugglinmike> jongund: Are we supposed to be applying the "use strict" directive to all JavaScript files? <jugglinmike> howard-e: Yes, that seems to be the case <jugglinmike> jongund: I also updated the JavaScript to replace all the "var" declarations with "let" declarations <jugglinmike> Matt_King: Should this be tested both with a mouse and with touch events, jongund? <jugglinmike> jongund: I only used "click" events, but probably we should test with both <jugglinmike> lola: I can volunteer if someone doesn't mind walking me through the changes that I'm looking for <jugglinmike> Matt_King: The kind of testing that we want to do is documented in a link in a comment at the top of the pull request <jugglinmike> lola: Okay, then, I can just use that <jugglinmike> Matt_King: The specific change that jongund made is described in the linked issue. What you're looking for in this functional review is the pointer behavior is as described in the issue across multiple browsers <jugglinmike> lola: Got it <jugglinmike> howard-e: I can perform the code review <jugglinmike> howard-e: jongund, you should be able to correct the formatting issues by running the command "npm run fix" <jugglinmike> lola: My GitHub handle is lolaodelola |
lolaodelola
approved these changes
Nov 18, 2024
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 works as expected on the browsers I was able to test on (I currently don't own Windows or Android devices):
- Safari on iOS and macOS
- Firefox on macOS
- Chrome on macOS
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3118
Updates the slider to:
const
andlet
instead ofvar
click
event on rail to move the minimum and maximum thumbsPreview
Preview updated multi-thumb slider example in compare branch
Review checklist
Reviewers: To learn what needs to be covered by each review, Follow the link for the type of review to which you are assigned.
Not applicable:
WAI Preview Link (Last built on Tue, 05 Nov 2024 19:15:57 GMT).