chore: always render ActionMenu in viewport when inside Dialog under feature flag#7524
chore: always render ActionMenu in viewport when inside Dialog under feature flag#7524francinelucca wants to merge 15 commits intomainfrom
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.
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>
|
👋 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.
Left a question/suggestion, approving in advance
| @@ -0,0 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
lol the filename, didn't know changesets was anti-marriage
…-display-in-viewport-enhancement
| const featureFlagDisplayInViewportInsidePortal = useFeatureFlag( | ||
| 'primer_react_action_menu_display_in_viewport_inside_dialog', | ||
| ) |
There was a problem hiding this comment.
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