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

feat: add carousel wc following basic and tabbed pattern #3438

Open
wants to merge 45 commits into
base: master
from

Conversation

@SethDonohue
Copy link
Collaborator

SethDonohue commented Jul 3, 2020

Description

Motivation & context

This adds the Carousel web component with both the Basic and Tabbed patterns of ARIA as defined here:
https://w3c.github.io/aria-practices/#carousel
https://w3c.github.io/aria-practices/#basic-carousel-elements
https://w3c.github.io/aria-practices/#tabbed-carousel-elements

It adds a Carousel Test Slide component for use in the Carousel Fixtures.

This ALSO required some minor non-breaking changes to Tabs listed below:

  • some methods and properties changed to public so they can be overridden when Tabs is extended
  • adds activeSlideIdChanged to properly handle the activeSlideId attr being updated

This ALSO requires changes to Flipper:

  • Now extends ARIAGlobalStatesAndProperties to allow for the use of aria-label and aria-labelledby

Questions: Please comment on this!

  • Should we expose methods for the author to implement their own custom methods for flexibility?

    • Autoplay, autoplay timer,
    • Rotation control (could have a public increment method that is built in for the custom Rotation control method to use)
    • Hover effect (typically used for pausing the slides when mouse is hovering over)
  • Or should all/any of the above methods baked in to the controller and strictly controlled per ARIA spec?

Notes:

  1. Currently these items are built in per ARIA spec:
  • The Carousel pauses rotation whenever the mouse is over the carousel OR the content is in focus.

  • Per ARIA spec it also stops rotation anytime keyboard focus is brought to any part of the carousel, AND is not restarted unless the user specifically activates the rotation control button.

    • The ARIA example does NOT do the above as it states to "restart the rotation once keyboard focus leaves the Carousel", so the ARIA spec/examples are in disagreement depending on how "not restarted unless the user specifically activates the rotation control button" is interpreted.
      With that in mind I have chosen to interpret it as to NOT restart the rotation when keyboard focus leaves as this may be a disruptive feature for AT users.
    • For this implementation, if the user has chosen to specifically restart the rotation the carousel stays rotating until the carousel is re-focused and the rotation control is triggered. This is my interpretation and I'd appreciate it if others chimed in on what they think is the right way to handle the spec. I plan to write to ARAI for clarification.
  1. The original FAST Carousel Spec stated that a reference to active slide would be passed when a change is emitted for the Author to read states like paused and activeid, but the active slide does not have access to these properties so the change emitted now passes a reference to the Carousel where those states can properly be read.

Issue type checklist

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

Is this a breaking change?

  • This change causes current functionality to break.

Adding or modifying component(s) in @microsoft/fast-components checklist

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.
@SethDonohue SethDonohue self-assigned this Jul 3, 2020
@SethDonohue SethDonohue changed the title feat: add carousel wc following basic and tabbed pattern ***WIP*** feat: add carousel wc following basic and tabbed pattern Jul 3, 2020
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-carousel-wc-useing-tabs branch 8 times, most recently from dfb329b to c20ad9b Jul 3, 2020
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-carousel-wc-useing-tabs branch from 4a4f2d7 to 2ef3091 Jul 13, 2020
@SethDonohue SethDonohue changed the title ***WIP*** feat: add carousel wc following basic and tabbed pattern feat: add carousel wc following basic and tabbed pattern Jul 13, 2020
@SethDonohue SethDonohue marked this pull request as ready for review Jul 13, 2020
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-carousel-wc-useing-tabs branch 4 times, most recently from c7f84ae to 1449ba4 Jul 13, 2020
@SethDonohue SethDonohue changed the title feat: add carousel wc following basic and tabbed pattern ***WIP*** feat: add carousel wc following basic and tabbed pattern Jul 14, 2020
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-carousel-wc-useing-tabs branch 3 times, most recently from 03a5296 to e594270 Jul 20, 2020
Seth Donohue added 24 commits Jul 3, 2020
… pattern, reordered for proper tab sequence, fixed tab not focusing on first keyboard increment
… tab sequence reorder, corrected change emitting
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-carousel-wc-useing-tabs branch from beda911 to b8f1c5a Aug 6, 2020
* HTML Attribute: loop
*/
@attr({ mode: "fromView" })
public loop: boolean = true;

This comment has been minimized.

Copy link
@janechu

janechu Aug 6, 2020

Member

Just catching this, I don't believe we can set attributes like this, you have to wait for the connectedCallback to initialize them. See #3494 as an example

@SethDonohue SethDonohue force-pushed the users/v-sedono/add-carousel-wc-useing-tabs branch from 81c1156 to 05f9474 Aug 6, 2020
Seth Donohue Seth Donohue
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-carousel-wc-useing-tabs branch from 05f9474 to 79fd4fd Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Components
  
In Review
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.