Skip to content

Refactor FilteredActionList to address a11y violations and use new ActionList. - #3247

Merged
radglob merged 53 commits into
mainfrom
refactor-filtered-action-list
May 26, 2023
Merged

Refactor FilteredActionList to address a11y violations and use new ActionList.#3247
radglob merged 53 commits into
mainfrom
refactor-filtered-action-list

Conversation

@radglob

@radglobradglob commented May 3, 2023

Copy link
Copy Markdown
Contributor

The FilteredActionList (and by extension SelectPanel) use the updated ActionList component.

Added a hideSelection prop that allows us to hide the Selection component that usually renders when an ActionList has a selectionVariant defined. The dropdown in the MarkdownEditor's saved replies did not show this previously, so this was added to keep the appearance the same.

Closeshttps://github.com/github/primer/issues/1861

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@radglob
radglob requested review from a team and siddharthkpMay 3, 2023 16:20
@changeset-bot

changeset-botBot commented May 3, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4740613

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

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

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

@github-actions

github-actionsBot commented May 3, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js101.6 KB (+0.12% 🔺)
dist/browser.umd.js102.16 KB (+0.15% 🔺)

@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 3, 2023 16:26 Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 3, 2023 16:27 Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 3, 2023 16:30 Inactive
@radglob
radglob temporarily deployed to github-pages May 3, 2023 17:31 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 3, 2023 17:31 Inactive
@radglob
radglob temporarily deployed to github-pages May 3, 2023 17:38 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 3, 2023 17:39 Inactive
@radglob
radglob temporarily deployed to github-pages May 3, 2023 17:57 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 3, 2023 17:57 Inactive
@radglob
radglob temporarily deployed to github-pages May 3, 2023 18:30 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 19, 2023 15:45 Inactive
@colebemis

Copy link
Copy Markdown
Contributor

@radglob Does this PR entirely close https://github.com/github/primer/issues/1861? It seems like it only address part of the issue:

  • The ActionList used in this component is deprecated and may need to be updated

I don't see code changes to address this items:

  • The aria-input-field-name rule corresponds to the item with role="listbox" which has no accessible label
  • This component seems to follow a combobox interaction pattern and requires a visible label for the input. The combobox pattern may address aria-input-field-name by providing aria-labelledby on the listbox pointed towards the input
  • The item with role="listbox" must have items with role="option"
  • This component may require a visual label which would mean design needs to be included in this issue

@radglob
radglob temporarily deployed to github-pages May 22, 2023 20:05 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 22, 2023 20:05 Inactive

@siddharthkpsiddharthkp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Final comments:

  • backward compatibility for showItemDividers
  • checking if there are no other backward breaking changes

Approving in advance

Comment threadsrc/SelectPanel/SelectPanel.features.stories.tsx
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 23, 2023 14:02 Inactive
@radglob
radglob temporarily deployed to github-pages May 23, 2023 14:06 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 23, 2023 14:06 Inactive
@radglob

Copy link
Copy Markdown
ContributorAuthor

@radglob Does this PR entirely close github/primer#1861? It seems like it only address part of the issue:

  • The ActionList used in this component is deprecated and may need to be updated

I don't see code changes to address this items:

  • The aria-input-field-name rule corresponds to the item with role="listbox" which has no accessible label
  • This component seems to follow a combobox interaction pattern and requires a visible label for the input. The combobox pattern may address aria-input-field-name by providing aria-labelledby on the listbox pointed towards the input
  • The item with role="listbox" must have items with role="option"
  • This component may require a visual label which would mean design needs to be included in this issue

Great point. I think I got mired in other issues and need to circle back to those. Should be easy enough to fix now that technical concerns are out of the way.

@radglob

Copy link
Copy Markdown
ContributorAuthor

@radglob Does this PR entirely close github/primer#1861? It seems like it only address part of the issue:

  • The ActionList used in this component is deprecated and may need to be updated

I don't see code changes to address this items:

  • The aria-input-field-name rule corresponds to the item with role="listbox" which has no accessible label
  • This component seems to follow a combobox interaction pattern and requires a visible label for the input. The combobox pattern may address aria-input-field-name by providing aria-labelledby on the listbox pointed towards the input
  • The item with role="listbox" must have items with role="option"
  • This component may require a visual label which would mean design needs to be included in this issue

Great point. I think I got mired in other issues and need to circle back to those. Should be easy enough to fix now that technical concerns are out of the way.

These are actually fixed. I think I was looking at a stale local storybook instance.

@radglob
radglob temporarily deployed to github-pages May 23, 2023 18:48 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 23, 2023 18:48 Inactive
@radglob
radglob temporarily deployed to github-pages May 25, 2023 15:35 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3247 May 25, 2023 15:36 Inactive
@radglob
radglob temporarily deployed to github-pages May 25, 2023 19:43 — with GitHub Actions Inactive
Sign up for freeto 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.

4 participants

@radglob@lesliecdubs@siddharthkp@colebemis