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

fix(breadcrumb): disabled behaviors and styles #2886

Merged

Conversation

marissahuysentruyt
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt commented Jul 3, 2024

Description

Disabled breadcrumbs (except for action buttons) in storybook still had pointer events, hover and focus styles. There was no indication that a text-based breadcrumb was disabled. This was partially because the tabindex was being set to 0, which then was adding specific styles. Additionally, the is-disabled class was not being added to the proper .spectrum-Breadcrumb-itemLink element. This PR should address these a11y concerns and disabled bugs.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

@jawinn

  • Verify that the disabled breadcrumb on the breadcrumb storybook page has no hover/focus styles, and no pointer events. Additionally, the breadcrumb item should be left out of the tab order
  • Verify that the parent .spectrum-Breadcrumbs-item does not have the is-disabled class
  • With the project running locally, (yarn start), find the breadcrumb storybook docs or default page
  • In breadcrumb/stories/breadcrumb.stories.js, add an additional disabled breadcrumb item (or feel free to experiment with ones already there). In the browser inspector, verify the following:
    • when isDisabled: false, the tabindex of your .spectrum-Breadcrumbs-itemLink breadcrumb is 0. It should be focusable, clickable, and have a hover state. The is-disabled class should not be applied at this point and there should not be a disabled attribute
    • when isDisabled: true, the tabindex should not be defined on that element. It should not be focusable, clickable, or have a hover state. The is-disabled class and aria-disabled="true" attribute should be added to .spectrum-Breadcrumbs-itemLink

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

Copy link

changeset-bot bot commented Jul 3, 2024

🦋 Changeset detected

Latest commit: a01ac07

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/breadcrumb Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jul 3, 2024

🚀 Deployed on https://pr-2886--spectrum-css.netlify.app

Copy link
Contributor

github-actions bot commented Jul 3, 2024

File metrics

Summary

Total size: 4.66 MB*
Total change (Δ): ⬆ 0.22 KB (0.00%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
breadcrumb 16.76 KB ⬆ 0.05 KB

Details

breadcrumb

File Head Base Δ
index-base.css 16.76 KB 16.71 KB ⬆ 0.05 KB (0.32%)
index-vars.css 16.76 KB 16.71 KB ⬆ 0.05 KB (0.32%)
index.css 16.76 KB 16.71 KB ⬆ 0.05 KB (0.32%)
metadata.json 9.78 KB 9.72 KB ⬆ 0.06 KB (0.57%)
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review July 3, 2024 16:22
@marissahuysentruyt marissahuysentruyt changed the title fix(breadcrumb): fix tabindex for disabled styles fix(breadcrumb): disabled behaviors and styles Jul 3, 2024
components/breadcrumb/stories/template.js Outdated Show resolved Hide resolved
components/breadcrumb/stories/template.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

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

Looks good. I would just add a changeset with a patch bump noting the CSS addition.

Copy link
Collaborator

@pfulton pfulton 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 taking care of this one!

- conditional tabindex for disabled styles (use ifDefined for tabindex
and aria-disabled)
- remove is-disabled class from spectrum-Breadcrumb-item (there were no
styles pertaining to this selector, so I removed the class from the list
item element in the template)
- use aria-disabled=true attribute selector
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-779-bug-breadcrumbs-disabled branch from 1e99cbd to a01ac07 Compare July 9, 2024 15:28
@marissahuysentruyt marissahuysentruyt merged commit a01ac07 into main Jul 9, 2024
11 checks passed
@marissahuysentruyt marissahuysentruyt deleted the marissahuysentruyt/css-779-bug-breadcrumbs-disabled branch July 9, 2024 15:47
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.

3 participants