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.

Sign up for GitHub

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

Jump to bottom

[TreeView] Mark leading action as private and add high-risk warning #5660

Merged
merged 9 commits into from
Feb 6, 2025
Merged

[TreeView] Mark leading action as private and add high-risk warning #5660

merged 9 commits into from
Feb 6, 2025

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented Feb 4, 2025 •
edited
Loading

Closes: https://github.com/github/accessibility-audits/issues/10114

Changelog

Changed

  • Add a warning banner to discourage usage of TreeView.LeadingAction sub component.
  • Move TreeView.LeadingAction to private to reduce discoverability while the component carries risk. There is prior art for marking a component as private, with Overlay.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Merge checklist

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 4, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 3a3b579

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 4, 2025 •
edited
Loading

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 105.42 KB (0%)
packages/react/dist/browser.umd.js 105.82 KB (0%)

@github-actions github-actions bot temporarily deployed to storybook-preview-5660 February 4, 2025 23:50 Inactive
TylerJDev
TylerJDev reviewed Feb 5, 2025
View reviewed changes
packages/react/src/TreeView/TreeViewWithLeadingAction.stories.tsx Outdated
Comment on lines 28 to 40
<Banner
title="High-risk feature"
description={
<p>
Usage of LeadingAction is discouraged due to high-accessibility risk. See
<Link inline={true} href="https://github.com/github/primer/issues/3446">
audit issue (staff only)
</Link>
.
</p>
}
variant="warning"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this ✨ I feel like we could make use of banners in a similar way in other stories that might be encouraging inaccessible patterns.. 👀

@khiga8 khiga8 added the skip changeset This change does not need a changelog label Feb 5, 2025
@khiga8 khiga8 changed the title Kh mark leading action as private Mark leading action as private and add high-risk warning Feb 5, 2025
@github-actions github-actions bot temporarily deployed to storybook-preview-5660 February 5, 2025 17:08 Inactive
@khiga8 khiga8 force-pushed the kh-mark-leading-action-as-private branch from 931d314 to 594c5eb Compare February 6, 2025 02:36
@khiga8 khiga8 changed the title Mark leading action as private and add high-risk warning [Treeview] Mark leading action as private and add high-risk warning Feb 6, 2025
@khiga8 khiga8 changed the title [Treeview] Mark leading action as private and add high-risk warning [TreeView] Mark leading action as private and add high-risk warning Feb 6, 2025
@khiga8 khiga8 force-pushed the kh-mark-leading-action-as-private branch from a035846 to a8f2d37 Compare February 6, 2025 02:39
@github-actions github-actions bot requested a deployment to storybook-preview-5660 February 6, 2025 02:40 Abandoned
@khiga8 khiga8 marked this pull request as ready for review February 6, 2025 02:42
@khiga8 khiga8 requested a review from a team as a code owner February 6, 2025 02:42
@khiga8 khiga8 requested a review from mperrotti February 6, 2025 02:42
@github-actions github-actions bot temporarily deployed to storybook-preview-5660 February 6, 2025 02:53 Inactive
@primer primer bot requested a review from a team as a code owner February 6, 2025 03:07
@github-actions github-actions bot temporarily deployed to storybook-preview-5660 February 6, 2025 03:08 Inactive
@primer primer bot temporarily deployed to github-pages February 6, 2025 03:22 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-5660 February 6, 2025 03:22 Inactive
TylerJDev
TylerJDev reviewed Feb 6, 2025
View reviewed changes
packages/react/src/TreeView/TreeViewWithLeadingAction.stories.tsx Outdated Show resolved Hide resolved
packages/react/src/TreeView/TreeView.docs.json Outdated
@@ -124,7 +124,7 @@
]
},
{
"name": "TreeView.LeadingVisual",
"name": "TreeView.LeadingVisual (Known accessiblity issues. Use at your own risk ⚠️)",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have the wrong leading component (LeadingAction), I actually didn't realize there was a LeadingVisual 😅

I'm wondering if we should just remove it docs for LeadingAction altogether? 🤔 Either way is good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I've decided to just remove it :)

@github-actions github-actions bot requested a deployment to storybook-preview-5660 February 6, 2025 14:13 Abandoned
@github-actions github-actions bot requested a deployment to storybook-preview-5660 February 6, 2025 14:23 Abandoned
@khiga8 khiga8 enabled auto-merge February 6, 2025 14:29
@khiga8 khiga8 added this pull request to the merge queue Feb 6, 2025
Merged via the queue into main with commit 6cd9b38 Feb 6, 2025
44 checks passed
@khiga8 khiga8 deleted the kh-mark-leading-action-as-private branch February 6, 2025 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerJDev TylerJDev TylerJDev approved these changes

@mperrotti mperrotti Awaiting requested review from mperrotti mperrotti is a code owner automatically assigned from primer/engineer-reviewers

Assignees
No one assigned
Labels
skip changeset This change does not need a changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@khiga8 @TylerJDev