open-sauced / app

🍕 Insights into your entire open source ecosystem.
https://pizza.new
Apache License 2.0
429 stars 228 forks source link

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

Closed FatumaA closed 1 month ago

FatumaA commented 2 months ago

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: https://github.com/open-sauced/app/issues/2264

Steps to reproduce

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

  1. components/atoms/ToggleGroup/toggle-group.stories.tsx
github-actions[bot] commented 2 months ago

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.

CorinaMurg commented 1 month ago

.take

CorinaMurg commented 1 month ago

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 commented 1 month ago

The only usage I see of the <ToggleGroup /> component is in https://github.com/open-sauced/app/blob/3950b24ade88b446812fa1cf38dd5d9b6ee464d2/components/molecules/NivoScatterChart/nivo-scatter-chart.tsx#L126-L145

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 commented 1 month ago

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 commented 1 month ago

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.

open-sauced[bot] commented 1 month ago

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

The release is available on GitHub release

Your semantic-release bot :package::rocket: