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

enhancement: a11y rule "anchor-is-valid" fails with a warning. #4136

Closed
Tracked by #4139
FatumaA opened this issue Sep 26, 2024 · 7 comments · Fixed by #4181
Closed
Tracked by #4139

enhancement: a11y rule "anchor-is-valid" fails with a warning. #4136

FatumaA opened this issue Sep 26, 2024 · 7 comments · Fixed by #4181
Assignees
Labels
a11y Issues related to accessibility 🐛 bug Something isn't working hacktoberfest Issues for https://hacktoberfest.digitalocean.com/ released on @beta

Comments

@FatumaA
Copy link
Contributor

FatumaA commented Sep 26, 2024

Describe the bug

jsx-a11y eslint plugin was recently added to the project and this rule - jsx-a11y/anchor-is-valid - fails with a warning.

This issue offers more context: #2264

Steps to reproduce

The following components and files are causing the rule to fail:

  1. components/atoms/ToggleGroup/toggle-group.stories.tsx
@FatumaA FatumaA added 🐛 bug Something isn't working 👀 needs triage labels Sep 26, 2024
Copy link
Contributor

Thanks for the issue, our team will look into it as soon as possible! If you would like to work on this issue, please wait for us to decide if it's ready. The issue will be ready to work on once we remove the "needs triage" label.

To claim an issue that does not have the "needs triage" label, please leave a comment that says ".take". If you have any questions, please comment on this issue.

For full info on how to contribute, please check out our contributors guide.

@nickytonline nickytonline added hacktoberfest Issues for https://hacktoberfest.digitalocean.com/ a11y Issues related to accessibility labels Oct 1, 2024
@CorinaMurg
Copy link
Contributor

.take

@CorinaMurg
Copy link
Contributor

I'm looking for some clarification re: the CustomItems story in components/atoms/ToggleGroup/toggle-group.stories.tsx.
Is the purpose to allow the children of ToggleGroup to be any type of HTML element/nested element, included a nested link?

CustomItems.args = {
  children: [
    <div key={1}>
      Option 1 w\{" "}
      <a href="#" target="_blank">
        link
      </a>
    </div>,
    <p key={2}>
      Option 2 w\ <strong>strong</strong>
    </p>,
    <div key={3}>
      Option 3 w\ <div>div</div>
    </div>,
  ],
  defaultSelection: "0",
  className: "w-max",
};

@nickytonline
Copy link
Member

The only usage I see of the <ToggleGroup /> component is in

<ToggleGroup handleChange={handleTogglePrFilter} className="hidden lg:flex" label="Pull Request State">
<>
All PRs
<span className="ml-2 py-0.5 px-1.5 h-fit bg-slate-200 text-slate-500 border rounded-full text-xs font-semibold">
{humanizeNumber(metadata.allPrs, null)}
</span>
</>
<>
Open
<span className="ml-2 py-0.5 px-1.5 h-fit bg-slate-200 text-slate-500 border rounded-full text-xs font-semibold">
{humanizeNumber(metadata.openPrs, null)}
</span>
</>
<>
Closed
<span className="ml-2 py-0.5 px-1.5 h-fit bg-slate-200 text-slate-500 border rounded-full text-xs font-semibold">
{humanizeNumber(metadata.closedPrs, null)}
</span>
</>
</ToggleGroup>

It looks like it can be an array of anything, but the items get wrapped in a <ToggleGroupItem />.

You can see this in action on our scatter chart here, e.g. https://app.opensauced.pizza/workspaces/0950814d-ed16-4201-add8-d17479cfdc1f/repository-insights/1588/activity

CleanShot 2024-10-21 at 12 55 08

@CorinaMurg
Copy link
Contributor

Thank you @nickytonline! Just wanted to make sure I understood the purpose for story 3 correctly.
Right now the linter is complaining that there's an href with invalid value. This is caused by a more serious issue, a link nested within one of the children of ToggleGroup. The children are basically buttons, and for accessibility we can't have an interactive element nested inside a button.

It's my first PR here so I want to make sure I stay within its scope. Would adjusting the types to make sure no interactive element is used still be within the scope?

@nickytonline
Copy link
Member

Yeah, let's remove that anchor tag with the href in the Storybook stories. More refactoring could be done, but let's leave it at that since the <ToggleGroupItem /> components are a button like you mentioned.

A future PR could have tooggle group items that are only text, but allow a pill as well, but again, for another day.

Copy link
Contributor

open-sauced bot commented Oct 25, 2024

🎉 This issue has been resolved in version 2.64.1-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Issues related to accessibility 🐛 bug Something isn't working hacktoberfest Issues for https://hacktoberfest.digitalocean.com/ released on @beta
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants