-
Notifications
You must be signed in to change notification settings - Fork 652
chore: always render ActionMenu in viewport when inside Dialog under feature flag #7524
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.
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
base: main
Are you sure you want to change the base?
chore: always render ActionMenu in viewport when inside Dialog under feature flag #7524
Conversation
🦋 Changeset detectedLatest commit: 1680a45 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
👋 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a feature-flagged behavior change so ActionMenu.Overlay can automatically opt into displayInViewport when rendered inside a portal (e.g., inside a Dialog), aiming to improve overlay positioning/clipping in portaled contexts.
Changes:
- Adds a new default feature flag:
primer_react_action_menu_display_in_viewport_inside_portal. - Updates
ActionMenu.Overlayto consult the feature flag and detect whether it is inside a portal to adjustdisplayInViewport. - Updates the
InsideDialogActionMenu story to enable the flag and remove the explicitdisplayInViewportprop.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/Portal/Portal.tsx | Attempts to ensure a “default” PortalContext exists so descendants can detect portal rendering. |
| packages/react/src/FeatureFlags/DefaultFeatureFlags.ts | Registers the new feature flag with a default value of false. |
| packages/react/src/ActionMenu/ActionMenu.tsx | Adds feature-flag logic and portal detection to influence displayInViewport. |
| packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx | Enables the feature flag in the InsideDialog story and relies on the new default behavior. |
|
@francinelucca I've opened a new pull request, #7525, to work on those changes. Once the pull request is ready, I'll request review from you. |
Updated the version of '@primer/react' from patch to minor and modified the ActionMenu rendering behavior.
…ithub.com:primer/react into chore/action-menu-display-in-viewport-enhancement
…e_portal feature flag (#7525) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/13307 |
siddharthkp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question/suggestion, approving in advance
| @@ -0,0 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol the filename, didn't know changesets was anti-marriage
…-display-in-viewport-enhancement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
| @@ -0,0 +1,3 @@ | |||
| export {Dialog} from './Dialog' | |||
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DialogContext is exported from Dialog.tsx but not re-exported from this index file. This is inconsistent with other similar contexts in the codebase (e.g., PortalContext in Portal/index.ts). For better API consistency and to avoid direct file imports, DialogContext should be re-exported here.
| export {Dialog} from './Dialog' | |
| export {Dialog, DialogContext} from './Dialog' |
| "@primer/react": minor | ||
| --- | ||
|
|
||
| chore: always render ActionMenu in viewport when inside Dialog under feature flag |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changeset description says "inside portal" but should say "inside Dialog" to match the PR title, description, and actual feature implementation. The feature flag name is primer_react_action_menu_display_in_viewport_inside_dialog which clearly indicates this is about Dialog, not Portal.
| import {isSlot} from '../utils/is-slot' | ||
| import type {FCWithSlotMarker, WithSlotMarker} from '../utils/types/Slots' | ||
| import {useFeatureFlag} from '../FeatureFlags' | ||
| import {DialogContext} from '../Dialog/Dialog' |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import should use '../Dialog' instead of '../Dialog/Dialog' to be consistent with other Dialog imports in this PR and throughout the codebase. However, this requires that DialogContext be re-exported from the Dialog index file first.
| import {DialogContext} from '../Dialog/Dialog' | |
| import {DialogContext} from '../Dialog' |
| const featureFlagDisplayInViewportInsidePortal = useFeatureFlag( | ||
| 'primer_react_action_menu_display_in_viewport_inside_dialog', | ||
| ) |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable name says "InsidePortal" but should say "InsideDialog" to match the feature flag name and the actual feature being implemented. The feature is about detecting if the ActionMenu is inside a Dialog, not inside a Portal.
Relates to https://github.com/github/primer/issues/6361
Implements @TylerDixon's suggestion to have the ActionMenu.Overlay be able to detect if its being rendered inside a Dialog (via a (new)
DialogContext) to then applydisplayInViewport=true. Adding this under a feature flag since its somewhat of a widespread change.Changelog
New
DialogContextto Dialogprimer_react_action_menu_display_in_viewport_inside_dialogfeature flag toDefaultFeatureFlags, enabling control over overlay display behavior inside dialogs.ActionMenu.Overlayto use the new feature flag and detect if it is inside a Dialog, modifying thedisplayInViewportprop logic accordingly.Changed
InsideDialogstory inFeatureFlagsand removed the explicitdisplayInViewportprop fromActionMenu.Overlay, relying on the feature flag for behavior.Removed
Rollout strategy
Testing & Reviewing
Merge checklist