Skip to content

Conversation

@TylerJDev
Copy link
Member

@TylerJDev TylerJDev commented Nov 13, 2025

Part of https://github.com/github/primer/issues/5900

  • Removes usingRemoveActiveDescendant in FilteredActionList.
    • We will remove primer_react_select_panel_remove_active_descendant once this is merged.
  • Adds private prop to FilteredActionList that allows configuration of focus management.

Changelog

New

  • Adds internal (private) prop to FilteredActionList

Removed

  • usingRemoveActiveDescendant logic removed from FilteredActionList

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

@changeset-bot
Copy link

changeset-bot bot commented Nov 13, 2025

🦋 Changeset detected

Latest commit: b44a0bc

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

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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-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 Nov 13, 2025
@github-actions
Copy link
Contributor

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

@github-actions github-actions bot requested a deployment to storybook-preview-7196 November 13, 2025 23:45 Abandoned
@primer-integration
Copy link

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

@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: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Nov 13, 2025
@TylerJDev TylerJDev changed the title SelectPanel: Focus Management FilteredActionList: Remove usingRemoveActiveDescendant logic Nov 14, 2025
@primer-integration
Copy link

🟢 ci completed with status success.

@TylerJDev TylerJDev marked this pull request as ready for review November 14, 2025 00:08
@TylerJDev TylerJDev requested a review from a team as a code owner November 14, 2025 00:08
Copilot finished reviewing on behalf of TylerJDev November 14, 2025 00:13
Copy link
Contributor

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 removes the primer_react_select_panel_remove_active_descendant feature flag from FilteredActionList and replaces it with a private prop _PrivateFocusManagement that allows internal configuration of focus management strategies (roving tabindex vs. active-descendant).

Key changes:

  • Feature flag logic replaced with a private prop-based approach
  • Test infrastructure updated to use prop-based configuration instead of feature flags
  • Simplified variable naming from usingRemoveActiveDescendant to usingRovingTabindex

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
packages/react/src/SelectPanel/SelectPanel.test.tsx Updated test infrastructure to use renderWithProp helper instead of feature flag wrapper
packages/react/src/FilteredActionList/useAnnouncements.tsx Replaced feature flag with focus management parameter
packages/react/src/FilteredActionList/FilteredActionList.tsx Added _PrivateFocusManagement private prop and removed feature flag dependency
.changeset/rotten-goats-stare.md Added changelog entry for minor release

describe('SelectPanel', () => {
it('should render an anchor to open the select panel using `placeholder`', () => {
renderWithFlag(<BasicSelectPanel />, usingRemoveActiveDescendant)
renderWithProp(<BasicSelectPanel />)
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Missing usingRemoveActiveDescendant parameter in renderWithProp call. All other test calls in the loop pass this parameter, so this call should pass it as well to ensure consistent testing of both focus management modes.

Suggested change
renderWithProp(<BasicSelectPanel />)
renderWithProp(<BasicSelectPanel />, usingRemoveActiveDescendant)

Copilot uses AI. Check for mistakes.
renderWithProp(
<>
<button type="button">outer button</button>
<BasicSelectPanel />
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Missing usingRemoveActiveDescendant parameter in renderWithProp call. This test case is inside the loop that iterates over both true and false values but doesn't pass the parameter to test both modes.

Suggested change
<BasicSelectPanel />
<BasicSelectPanel usingRemoveActiveDescendant={usingRemoveActiveDescendant} />

Copilot uses AI. Check for mistakes.
Comment on lines +237 to 240
renderWithProp(
<SelectPanel
onOpenChange={() => {}}
onFilterChange={() => {}}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Missing usingRemoveActiveDescendant parameter in renderWithProp call. This test is inside the loop that tests both focus management modes, but the parameter isn't being passed.

Copilot uses AI. Check for mistakes.
@TylerJDev TylerJDev added this pull request to the merge queue Nov 14, 2025
@TylerJDev TylerJDev removed this pull request from the merge queue due to a manual request Nov 14, 2025
@TylerJDev TylerJDev added this pull request to the merge queue Nov 14, 2025
Merged via the queue into main with commit 70f5ffe Nov 14, 2025
60 checks passed
@TylerJDev TylerJDev deleted the focus-management-filtered-actionlist branch November 14, 2025 17:54
@primer primer bot mentioned this pull request Nov 14, 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.

3 participants