-
Notifications
You must be signed in to change notification settings - Fork 350
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
Conversation
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 |
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
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.
@jongund code changes look good to me!
Unrelated as this PR didn't introduce the issue. It seems there's a visual glitch when this is on the deployed environment. Being that the ring shown around the min and max thumb slider is slightly cut off at the top on hover. Example image below:
Looking through the code, it seems the y
attribute being set for minSliderFocusNode
and maxSliderFocusNode
should be a minimum value of 2
. But in the deployed environment, this gets calculated to a value of -4
Since this change didn't explicitly warrant an in-depth visual review, I created #3174 to track that separately.
@a11ydoer will test android device with tabbing of the rail - functional test only. |
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).