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 18 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 72fd11f 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>
siddharthkp
left a comment
There was a problem hiding this comment.
Left one clarifying question, everything else looks great! Approving in advance!
| variant={variant} | ||
| displayInViewport={displayInViewport} | ||
| displayInViewport={ | ||
| displayInViewport !== undefined ? displayInViewport : featureFlagDisplayInViewportInsidePortal && isInsideDialog |
There was a problem hiding this comment.
Interesting that we have this in props and automatically applied from context. Should we remove it from props or do we want to preserve displayInViewport=false as an override?
There was a problem hiding this comment.
I was wanting to let the gate open for someone to want to do displayInViewport=true ad-hoc. (like if your ActionMenu is not inside a Dialog, but you still want to force it to displayInViewPort. A good use-case I see for this would be the "close as" - completed, duplicate - not planned - menu for issues when inside the sidebar - it works well now due to some marie-fu, but this'd be a much cleaner solution - ).
And then I got to thinking that if we allow displayInViewport=true, then we should probably allow displayInViewport=false to override the default behavior, hence this seemingly weird conditional. Thoughts?
|
@francinelucca I've opened a new pull request, #7565, to work on those changes. Once the pull request is ready, I'll request review from you. |
…rtal (#7565) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com>
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