Skip to content

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Oct 9, 2025

Closes https://github.com/github/primer/issues/5180

Using the AnchoredOverlay component with its renderAnchor prop rendering an IconButton with the prop spread and aria-label="whatever" results in some TypeScript error. Adding the prop aria-labelledby={undefined} is a workaround that fixes this error.

The error was caused by a type conflict with the IconButton component's accessibility props. The IconButton component has a strict type constraint requiring either aria-label OR aria-labelledby (but not both). When the AnchoredOverlay's renderAnchor function passes props that might include aria-labelledby, and we also explicitly provide aria-label="Info", TypeScript detected a potential violation of this constraint.

Changelog

New

Changed

AnchoredOverlay, omit aria-label and aria-labelledby from renderAnchor props

Removed

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

Testing & Reviewing

Merge checklist

@jonrohan jonrohan requested a review from a team as a code owner October 9, 2025 21:57
@jonrohan jonrohan requested review from Copilot and hectahertz October 9, 2025 21:57
Copy link

changeset-bot bot commented Oct 9, 2025

🦋 Changeset detected

Latest commit: 4ab0cf2

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

This PR includes changesets to release 2 packages
Name Type
@primer/react Patch
@primer/styled-react 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

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a TypeScript type conflict in the AnchoredOverlay component where the renderAnchor prop would pass accessibility properties that could conflict with IconButton's strict accessibility requirements. The IconButton component requires either aria-label OR aria-labelledby but not both, and the previous implementation could cause type errors when both were present.

Key Changes

  • Modified the renderAnchor prop type to exclude aria-label and aria-labelledby from the props passed to the anchor element
  • Added a type test to verify the fix works correctly with IconButton components

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
AnchoredOverlay.tsx Updated renderAnchor prop type to omit aria-label and aria-labelledby from passed props
AnchoredOverlay.types.test.tsx Added type test to verify IconButton with aria-label works without TypeScript errors

@github-actions github-actions bot added staff Author is a staff member integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Oct 9, 2025
Copy link
Contributor

github-actions bot commented Oct 9, 2025

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

@github-actions github-actions bot requested a deployment to storybook-preview-6973 October 9, 2025 22:01 Abandoned
@pksjce
Copy link
Contributor

pksjce commented Oct 10, 2025

Do you think the requirement is more like having either one of aria-label or aria-labelledby ? In this case, renderAnchor wouldn't accept aria-label as a prop right?

Copy link
Contributor

@hectahertz hectahertz left a comment

Choose a reason for hiding this comment

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

👍

@jonrohan
Copy link
Member Author

Do you think the requirement is more like having either one of aria-label or aria-labelledby ? In this case, renderAnchor wouldn't accept aria-label as a prop right?

From what I could tell we weren't using these labels in the renderAnchor props, they were inherited by declaring all html attributes as the type. It is the IconButton's requirements that we use one or the other. And these are controlled by the user so I think omitting the type from the spread is valid fix here.

@github-actions github-actions bot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Oct 13, 2025
@primer-integration
Copy link

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/4617

@primer-integration
Copy link

🟢 ci completed with status success.

@github-actions github-actions bot added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: failing Changes in this PR cause breaking changes in gh/gh labels Oct 15, 2025
@jonrohan jonrohan added this pull request to the merge queue Oct 15, 2025
Merged via the queue into main with commit a3f7ea9 Oct 15, 2025
43 checks passed
@jonrohan jonrohan deleted the anchored_overlay_aria_props branch October 15, 2025 21:00
@primer primer bot mentioned this pull request Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh staff Author is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants