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(toggle-button): new component #2061

Merged
merged 21 commits into from
Jun 15, 2023
Merged

Conversation

ArtBlue
Copy link
Contributor

@ArtBlue ArtBlue commented May 12, 2023

Fixes #1970

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

This introduces the Toggle Button component which is a subset of a future component, Toggle Button Group (responsive layouts using a set of Toggle Buttons).

Notes

Screenshots

image

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@ArtBlue ArtBlue self-assigned this May 12, 2023
@ArtBlue
Copy link
Contributor Author

ArtBlue commented May 12, 2023

I had doubts about the benefits of having the buttons in a <ul> (deviation from the current draft of MIND patterns). From a screen reader perspective, there was a single spoken "list" between the two versions:

WITH UL

eBay Skin  document   
heading    level 2  @ebay/skin/toggle-button-group   The Toggle Button Group is a group of single or multi-toggle buttons providing a visual emphasis on the available selectable choices.  Toggle button group of text items  grouping  list  toggle button  not pressed    Text Button 1      toggle button  pressed    Text Button 2      toggle button  not pressed    Text Button 3    

WITHOUT UL

eBay Skin  document   
heading    level 2  @ebay/skin/toggle-button-group   The Toggle Button Group is a group of single or multi-toggle buttons providing a visual emphasis on the available selectable choices.  Toggle button group of text items  grouping    toggle button  not pressed    Text Button 1      toggle button  pressed    Text Button 2      toggle button  not pressed    Text Button 3    

Copy link
Contributor

@saiponnada saiponnada left a comment

Choose a reason for hiding this comment

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

Regarding semantics of using UL LI or not, when there is a button group there needs to be some level of defining a group. It can be either

  • Using ul li as shown in mind patterns (gives list) (or)

  • Assuming you are using role=group and aria-labelledby attribute to achieve that

    <div role="group" aria-labelledby="toggle-button-group-label">
       <span id="toggle-button-group-label">Toggle Button Group</span>
       <button type="button" class="toggle-button" aria-pressed="true">Image Button Title 1</button>
       <button type="button" class="toggle-button">Image Button Title 2</button>
       <button type="button" class="toggle-button">Image Button Title 3</button>
    </div>
    

IMO, both of these approaches are fine. The choice between them depends on the desired level of semantic markup and flexibility in styling.

I had doubts about the benefits of having the buttons in a <ul> (deviation from the current draft of MIND patterns). From a screen reader perspective, there was a single spoken "list" between the two versions:

@ianmcburnie
Copy link
Contributor

1st rule of ARIA is don't use ARIA ;-)

The first rule of ARIA is if a native HTML element or attribute has the semantics and behavior you require, use it instead of re-purposing an element and adding ARIA.

@agliga
Copy link
Contributor

agliga commented May 16, 2023

Lets leave this for 16.3. That way we can have this as the first change and then also get a component out in the same release.

@ArtBlue
Copy link
Contributor Author

ArtBlue commented May 16, 2023

@saiponnada , @ianmcburnie , just to clarify, the a11y issues to resolve for this particular PR is only for guidance to teams using the toggle-button component since it won't currently come wrapped in toggle-button-group since the work has been split up. In a future PR, when toggle-button-group is released, we will utilize our own guidance.

Originally, I believe we discussed having role="group" as well as aria-label. I may have seen both of those and the <ul> implemented at some point (I don't now). Sure, we can definitely use the <ul> in lieu of role="group", but we will still need to retain the aria-label to describe the list of buttons.

For now, this is what it would look like:

<div class="toggle-button-group">
    <ul aria-label="List of Shipping Options">
        <li>
            <button type="button" aria-pressed="false">
                IMAGE
                UPS Ground
                PRICE
            </button>
        <li>
            <button type="button" aria-pressed="false">
                IMAGE
                USPS Priority
                PRICE
            </button>
        <li>
            <button type="button" aria-pressed="true">
                IMAGE
                Fedex Ground
                PRICE
            </button>
        </li>
    </ul>
</div>

Would that work? I added the label on the <ul> to add context to the list instead of the parent <div>. What do you think?

@saiponnada
Copy link
Contributor

I guess that should work. By default UL LI tag is interpreted as List. So in the aria label, just Shopping options is enough. Sometimes if we use a heading e.g, Shopping options, then this would be unnecessary. It depends on the context around. So when we develop this in ebay UI label might be empty.

@ArtBlue
Copy link
Contributor Author

ArtBlue commented May 18, 2023

I guess that should work. By default UL LI tag is interpreted as List. So in the aria label, just Shopping options is enough. Sometimes if we use a heading e.g, Shopping options, then this would be unnecessary. It depends on the context around. So when we develop this in ebay UI label might be empty.

Sounds good! Yea, I was a bit concerned over the redundancy.

I do think that the aria-label should be necessary though for context to let users know they can select/toggle the buttons inside. Actually, not just for context, but because these buttons can either be multi-select or single-select, and users won't know that when interacting with the buttons. I don't think aria has anything out of the box to relay that info in a different way.

Something like, "Single-selection group of shipping options" (single-select) and "Multi-selection group of size options" (multi-select).

@agliga agliga changed the base branch from 16.2.0 to 16.3.0 May 19, 2023 18:12
@ArtBlue ArtBlue marked this pull request as ready for review May 22, 2023 15:49
@ianmcburnie
Copy link
Contributor

but we will still need to retain the aria-label to describe the list of buttons

I was imagining the context for the button group would be provided by either:

a) a preceding onscreen heading element (e.g. "Shipping Options")
b) quickly deducible from button labels alone (e.g. from "Visa", "Mastercard" it can be deduced these are payment options)

Option a) would always be preffered for most clarity.

The trouble with aria-label on the list or container is that it creates a different experience for AT. Better if we can to have same experience for all users.

@ArtBlue
Copy link
Contributor Author

ArtBlue commented May 23, 2023

I was imagining the context for the button group would be provided by either:

a) a preceding onscreen heading element (e.g. "Shipping Options") b) quickly deducible from button labels alone (e.g. from "Visa", "Mastercard" it can be deduced these are payment options)

Option a) would always be preffered for most clarity.

The trouble with aria-label on the list or container is that it creates a different experience for AT. Better if we can to have same experience for all users.

I see. In the scenario without the heading though, would the context really be clear from the buttons themselves? One of the minimal layout themes simply has numbers -shoe sizes. Would we just prescribe/require a heading in those cases? I'm fine either way; we just have to cover all the use-cases and be more prescriptive in some circumstances.

That said, I'm not sure if there will be anything applicable to this PR. Should we continue this discussion in the following PR where this structure is going to be implemented (toggle-button-group)? This PR is merely for the individual toggle-button.

@ianmcburnie
Copy link
Contributor

ianmcburnie commented May 23, 2023

Would we just prescribe/require a heading in those cases?

Preference for app teams would be this order:

  1. Use onscreen heading
  2. Use offscreen heading
  3. Use aria-label. This needs to go on an element with a role that supports aria-label. Options are a) the list b) the container div with role=group. I would also just keep the label to "Shipping Options" rather than "List of Shipping Options".

We could document this on Skin, and rather than having all 3 examples shown on Skin we could just go with the minimal version (option 3, ARIA) for the purposes of keeping examples concise (we do this with radio button and checkbox modules).

Copy link
Member

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few comments and points for consideration.

docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
src/less/toggle-button/toggle-button.less Outdated Show resolved Hide resolved
dist/toggle-button/toggle-button.css Outdated Show resolved Hide resolved
docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
docs/src/js/main.js Show resolved Hide resolved
@ianmcburnie
Copy link
Contributor

I would add a paragraph of text after the heading here explaining very briefly what "list view" means and under what circumstances it is used. Same goes for "gallery". These terms have connotations with List and Gallery views on SRP, which are completely different, so I think there could be some confusion here.

To be consistent with rest of docs, I would also separate out the second multiline example. Same goes for the image and icon examples for gallery view.

Screen Shot 2023-05-25 at 11 43 52 AM Screen Shot 2023-05-25 at 12 08 47 PM

@ianmcburnie
Copy link
Contributor

It feels like the List View variations section repeats the example that was in the first toggle button section. Following on from my previous comment, I wonder if we could just merge these two sections and show all list view variations in sequence, one at a time, followed by all gallery view variations?

Screen Shot 2023-05-25 at 2 48 27 PM Screen Shot 2023-05-25 at 2 47 46 PM

@ianmcburnie
Copy link
Contributor

ianmcburnie commented May 26, 2023

Let's try and add some flavorful text after each h3. One thing I like to do is highlight what the delta is from the previous example.

For example:

Toggle Button with Title and Subtitle: a subtitle can be added via the toggle-button__subtitle block.
Toggle Button with Title and Multiline Subtitle: use paragraphs to create a subtitle that spans multiple lines (note: I'd even be tempted to removed this as a heading and just have it fall under the preceding heading
Toggle Button with Icon, Title, and Subtitle: Any 16x16 svg icon can be dropped in, inside of a toggle-button_
icon block.

EDIT: I see we are lacking this in other places, like split-button and listbox-button. So we should plan on doing a sweep of those in a follow up PR to restore consistency to the docs.

Screen Shot 2023-05-26 at 1 56 15 PM

@ArtBlue
Copy link
Contributor Author

ArtBlue commented May 30, 2023

Let's try and add some flavorful text after each h3.

@ianmcburnie , done.

@ArtBlue ArtBlue force-pushed the 1970-selection-cell-new-component branch from 3a09804 to 4ca7368 Compare June 8, 2023 14:00
Copy link
Member

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Looks great! Just one small docs change and then I think this should be good to merge

docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
LuLaValva

This comment was marked as duplicate.

Copy link
Contributor

@ianmcburnie ianmcburnie left a comment

Choose a reason for hiding this comment

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

Just a few docs change suggestions. If you like them, they can be committed directly here in the GitHub UI.

docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
@ArtBlue ArtBlue merged commit d269924 into 16.3.0 Jun 15, 2023
@ArtBlue ArtBlue deleted the 1970-selection-cell-new-component branch June 15, 2023 13:45
ArtBlue added a commit that referenced this pull request Jun 22, 2023
* feat(toggle-button): new component
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.

5 participants