-
Notifications
You must be signed in to change notification settings - Fork 355
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
Color Viewer Slider Example Update (slider-1) #1472
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.
code review: I only have SUPER minor comments on the code review, this looks really good! But please do delete the old javascript and css files (slide.js and slider.css) in this PR, and consider the other comments. This is otherwise an approving code review.
test review: The tests as they are look really good, but it does seems like there is a quite a bit of mouse-user functionality that is not tested. We aren't requiring tests of mouse behavior that in the test review right now, but maybe we should make a backlog task for this to not loose track of untested functionality for these widgets?
examples/slider/slider-1.html
Outdated
@@ -2,7 +2,7 @@ | |||
<html lang="en"> | |||
<head> | |||
<meta charset="utf-8" /> | |||
<title>Horizontal Slider Example | WAI-ARIA Authoring Practices 1.2</title> | |||
<title>Color Picker Slider Example Using SVG | WAI-ARIA Authoring Practices 1.1</title> |
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.
Is this really 1.1?
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.
fixed
examples/slider/js/slider-1.js
Outdated
|
||
ColorPickerSliders.prototype.moveSliderTo = function (slider, value) { | ||
|
||
console.log('[moveSliderTo][slider]: ' + slider.sliderNode.classList); |
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 console log too -- if it's just for debugging I'd remove it!
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.
fixed
@spectranaut |
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 great! :)
examples/slider/css/slider-1.css
Outdated
text-align: center; | ||
padding: 0.25em; | ||
} | ||
|
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.
Suggest adding
.color-box {
forced-color-adjust: none;
}
so the color box renders the actual color even in high contrast mode.
examples/slider/slider-1.html
Outdated
<h2 id="id-color-picker">Color Viewer</h2> | ||
<div id="id-red" class="label">Red</div> | ||
<div class="color-group red"> | ||
<button class="change dec10" aria-label="decrement red ten" tabindex="-1"> |
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.
Would aria-label be better as "decrease red by 10"? (and similar for all the others?)
Regression test coverage:Examples without any regression tests:
Examples missing some regression tests:
Example pages with Keyboard or Attribute table rows that do not have data-test-ids:
SUMMARY:56 example pages found. ERROR - missing tests: Please write missing tests for this report to pass. |
@spectranaut @nschonni @mcking65 |
Run |
@nschonni |
@nschonni |
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.
Tested with Windows 10, NVDA 2020.3, JAWS 2020.2012.13; Firefox 84.0.1 and Chrome 87.0.4280.141. All worked as expected as far as documented accessibility functionality, keyboard interaction, Windows HCM, and screen readers were concerned.
@carmacleod, could I tag you in for MacOS a11y tests? My test Mac is with another co-worker at the moment.
Since @charmarkk did functionality review agaist on non iOS devices, I only check color picker slider example with mobile support with iphone, iOS VoiceOver The slider itself works well by annoucing current value or changed value with gesture (See the screen shot with VoiceOver text output). Question is on increase/decrease button. For example, when "increase red by ten" is touched with gesture, the value changed accordingly. (See the screen shot with VoiceOver text output). However, the changed value itself is not announced although visual user can see the color change in the Color view box. @jongund Should we announce changed current value with button action or it is not necessary? |
You deleted files so the index.html is out out of sync. Last time it got regenerated by the pre-commit hook during an unrelated changed, but you can run the script from the previous comment to do it |
@mcking65 |
This pull request is superseded by pull #1746. |
This is an updated version of the slider 1 example for picking a color.
New features include:
Preview Link
Color Viewer Slider Example
Review checklist