Skip to content
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

Add tabbed carousel example for issue 955 #1120

Merged
merged 131 commits into from
Apr 30, 2020

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Aug 2, 2019

To resolve #955, adds an image carousel that includes a tablist control.

Preview Link

Preview example carousel in the compare branch of this pull request

Review checklist


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Apr 28, 2020, 6:15 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL


😭  Sorry, there was an error generating the HTML. Please report this issue!
�[36mSpecification: https://rawcdn.githack.com/w3c/aria-practices/2a9d5bdf3a03e234dfb33267a05ebf68887b9e70/aria-practices.html?isPreview=true&publishDate=2020-04-28�[39m
�[36mReSpec version: 25.6.0�[39m
�[36mFile a bug: https://github.com/w3c/respec/�[39m
�[36mError: Error: Evaluation failed: Timeout: document.respecIsReady didn't resolve in 27213ms.�[39m
�[36m    at ExecutionContext._evaluateInternal (/u/spec-generator/node_modules/puppeteer/lib/ExecutionContext.js:122:13)�[39m
�[36m    at runMicrotasks (<anonymous>)�[39m
�[36m    at processTicksAndRejections (internal/process/task_queues.js:97:5)�[39m
�[36m    at async ExecutionContext.evaluate (/u/spec-generator/node_modules/puppeteer/lib/ExecutionContext.js:48:12)�[39m
�[36m    at async generateHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:128:12)�[39m
�[36m    at async fetchAndWrite (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:95:18)�[39m
�[36m    at async Object.generate [as respec] (/u/spec-generator/generators/respec.js:14:20)�[39m
�[36m    at async generate (/u/spec-generator/server.js:91:29)�[39m
�[36m  -- ASYNC --�[39m
�[36m    at ExecutionContext.<anonymous> (/u/spec-generator/node_modules/puppeteer/lib/helper.js:111:15)�[39m
�[36m    at DOMWorld.evaluate (/u/spec-generator/node_modules/puppeteer/lib/DOMWorld.js:112:20)�[39m
�[36m    at runMicrotasks (<anonymous>)�[39m
�[36m    at processTicksAndRejections (internal/process/task_queues.js:97:5)�[39m
�[36m  -- ASYNC --�[39m
�[36m    at Frame.<anonymous> (/u/spec-generator/node_modules/puppeteer/lib/helper.js:111:15)�[39m
�[36m    at Page.evaluate (/u/spec-generator/node_modules/puppeteer/lib/Page.js:860:43)�[39m
�[36m    at Page.<anonymous> (/u/spec-generator/node_modules/puppeteer/lib/helper.js:112:23)�[39m
�[36m    at generateHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:128:23)�[39m
�[36m    at runMicrotasks (<anonymous>)�[39m
�[36m    at processTicksAndRejections (internal/process/task_queues.js:97:5)�[39m
�[36m    at async fetchAndWrite (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:95:18)�[39m
�[36m    at async Object.generate [as respec] (/u/spec-generator/generators/respec.js:14:20)�[39m
�[36m    at async generate (/u/spec-generator/server.js:91:29)�[39m
�[36m�[39m

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

jongund and others added 30 commits March 18, 2017 07:07
initial draft of design pattern for carousel for issue #43.
Updated Carousel Design Pattern and Examples
Merge branch 'master' into carousel-v2
Comment on lines 52 to 68
<label for="accessible-controls">
<input type="checkbox" id="accessible-controls" value="moreaccessible" aria-describedby="accessible-controls-desc">
Tablist carousel controls and captions displayed before and/or after the image.
</label>
<p id="accessible-controls-desc">This option is more accessible than rendering controls and captions on top of the image, because the controls are harder to perceive and captions are more difficult to read with rotating images behind them.</p>

<label for="rotation-paused">
<input type="checkbox" id="rotation-paused" value="paused" aria-describedby="rotation-paused-desc">
Auto-rotation is initially paused (<a href="javascript:history.go(0)"><img class="reload" src="images/reload-icon.png" alt="">reload needed</a>).
</label>
<p id="rotation-paused-desc">This option controls whether the carousel is paused or playing on page load. The paused option improves accessibility for users with visual impairments and people who are distracted or confused by auto-rotation, but allows users to start auto-rotation using the start/stop button. If this option is changed, the page must be reloaded to see the effect on behavior.</p>

<label for="rotation-disabled">
<input type="checkbox" id="rotation-disabled" value="norotate" aria-describedby="rotation-disabled-desc">
Auto-rotation is disabled.
</label>
<p id="rotation-disabled-desc">This option improves accessibility for users with visual impairments and people who are distracted or confused by auto-rotation by disabling the auto-rotation feature and removing the start/stop button from the user interface. Users can use the tablist to manually navigate through the slides.</p>
Copy link
Contributor

@carmacleod carmacleod Apr 21, 2020

Choose a reason for hiding this comment

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

Some new words for the checkboxes. Followed N&N Guideline:

  1. Use positive and active wording for checkbox labels

Also, put back "checked" defaults for the first 2 checkboxes.

Suggested change
<label for="accessible-controls">
<input type="checkbox" id="accessible-controls" value="moreaccessible" aria-describedby="accessible-controls-desc">
Tablist carousel controls and captions displayed before and/or after the image.
</label>
<p id="accessible-controls-desc">This option is more accessible than rendering controls and captions on top of the image, because the controls are harder to perceive and captions are more difficult to read with rotating images behind them.</p>
<label for="rotation-paused">
<input type="checkbox" id="rotation-paused" value="paused" aria-describedby="rotation-paused-desc">
Auto-rotation is initially paused (<a href="javascript:history.go(0)"><img class="reload" src="images/reload-icon.png" alt="">reload needed</a>).
</label>
<p id="rotation-paused-desc">This option controls whether the carousel is paused or playing on page load. The paused option improves accessibility for users with visual impairments and people who are distracted or confused by auto-rotation, but allows users to start auto-rotation using the start/stop button. If this option is changed, the page must be reloaded to see the effect on behavior.</p>
<label for="rotation-disabled">
<input type="checkbox" id="rotation-disabled" value="norotate" aria-describedby="rotation-disabled-desc">
Auto-rotation is disabled.
</label>
<p id="rotation-disabled-desc">This option improves accessibility for users with visual impairments and people who are distracted or confused by auto-rotation by disabling the auto-rotation feature and removing the start/stop button from the user interface. Users can use the tablist to manually navigate through the slides.</p>
<label for="accessible-controls">
<input type="checkbox" id="accessible-controls" value="moreaccessible" aria-describedby="accessible-controls-desc" checked>
Display controls and captions outside of image
</label>
<p id="accessible-controls-desc">This option is more accessible than rendering controls and captions within the image, because controls are easier to perceive and captions are easier to read without rotating images behind them.</p>
<label for="rotation-paused">
<input type="checkbox" id="rotation-paused" value="paused" aria-describedby="rotation-paused-desc" checked>
Pause auto-rotation on load (<a href="javascript:history.go(0)"><img class="reload" src="images/reload-icon.png" alt="">reload needed</a>).
</label>
<p id="rotation-paused-desc">This option controls whether the carousel is paused or playing on page load. The paused option improves accessibility for users with visual impairments and people who are distracted or confused by auto-rotation, but allows users to start auto-rotation using the start/stop button.</p>
<label for="rotation-disabled">
<input type="checkbox" id="rotation-disabled" value="norotate" aria-describedby="rotation-disabled-desc">
Disable auto-rotation
</label>
<p id="rotation-disabled-desc">This option improves accessibility for users with visual impairments and people who are distracted or confused by auto-rotation by disabling the auto-rotation feature and removing the start/stop button from the user interface. Users can use the tablist to manually navigate through the slides.</p>

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think the description text should be indented to align vertically with the label text.

Also, other settings pages that use checkboxes with descriptions usually make the description text lighter than the label text, so we could make the label text bold, so that it is more clearly the "call to action".

@jongund
Copy link
Contributor Author

jongund commented Apr 21, 2020

@carmacleod

  1. Updated focus styling to be solid, like new chrome focus styling
  2. Updated checkbox options with CM suggested changes
  3. Initialization of paused states now includes operating system preferences for reduced motion or animations

@smhigley
Copy link
Contributor

@jongund I liked your suggestion and updates to make the behavior options less hard-coded, and I had a thought to make it even easier to update and change, and bring the coding style more in line with common UI component patterns.

I made an update to the way the defaults are checked and coded that I think simplifies the code even further (keeping the same behavior where it respects the checkbox settings in the HTML + the URL params).

The second change I made was just to move the prefers-reduced-motion logic directly into the carousel component, so anyone who copies the code will get that behavior.

@jongund
Copy link
Contributor Author

jongund commented Apr 21, 2020

@smhigley
@mcking65

There needs to be some coordination on this pull request, I have been working on making changes based on today's call and all of a sudden my changes have been overridden and a bunch of files I have a separate pull request for the other carousel have been changed in this pull request.

@jongund
Copy link
Contributor Author

jongund commented Apr 22, 2020

@mcking65
@carmacleod
@smhigley

There are changes to some files in this pull request that probably should not be included:

  1. aria-practices.html
  2. carousel-1.html
  3. test/tests/carousel_carousel-1.js

I am not sure how these files got changed in this pull request, and it was most likely me since this pull request is been around for along time (7 months). I think this pull request should be limited to just adding the tabbed carousel example, so I would like to remove the changes to these files from the pull request. Since they are minor changes anyway and I have other pull requests that address the issues in these changes.

Thank you especially to Sara and Carolyn for all the work you have done in improving the tabbed carousel example these past few months, looking forward to getting this merged soon.

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

Thanks for the nice focus ring, @jongund - looks great!

I think this is almost good to go now. I only have 2 comments:

  1. Please add the note about reduced motion system settings that was suggested in Add tabbed carousel example for issue 955 #1120 (comment) [Edit: I went ahead and committed this for you because we said it was missing at the last call]
  2. Why do all of the anchors have href="#!"? I don't know what that does. Should they just be href="#"?

Giving +1 assuming that those 2 comments will be addressed.

@carmacleod
Copy link
Contributor

@jongund I pushed 2 minor changes in:

  • Add note about reduced motion setting to "pause on load" checkbox
  • Remove periods from checkbox labels

Hope that's ok.

So the only thing I'm still wondering about is the href="#!".

@jongund
Copy link
Contributor Author

jongund commented Apr 28, 2020

@carmacleod

I fixed the hrefs to use #, not sure where the #! came from.

Thanks for adding the note on the pause at start up.

Jon

@mcking65 mcking65 merged commit b340330 into master Apr 30, 2020
@mcking65 mcking65 deleted the issue955-add-tabbed-carousel-example branch April 30, 2020 06:59
@mcking65
Copy link
Contributor

@jongund, thank you, thank you for all your work on this!!

michael-n-cooper pushed a commit that referenced this pull request Apr 30, 2020
Add tabbed carousel example (pull #1120)

To resolve #955, adds an example of a carousel that uses the tabs pattern as carousel controls.

Co-authored-by: Matt King <a11yThinker@Gmail.com>
Co-authored-by: Scott Vinkle <scott.vinkle@shopify.com>
Co-authored-by: Valerie Young <valerie@bocoup.com>
Co-authored-by: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
Co-authored-by: Sarah Higley <sarah.higley@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add carousel example with tabs as slide control