Revert "AnchoredOverlay: Add support for CSS anchor positioning (#7604)"#7657
Revert "AnchoredOverlay: Add support for CSS anchor positioning (#7604)"#7657francinelucca merged 1 commit intomainfrom
Conversation
This reverts commit 61ae74c.
|
|
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/15847 |
|
Integration test results from github/github-ui:
VRT check ensures that when visual differences are detected, the PR cannot proceed until someone acknowledges the changes by adding the "visual difference acknowledged" label. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
There was a problem hiding this comment.
Pull request overview
This PR reverts the previously-added CSS anchor positioning support for AnchoredOverlay, removing the feature-flagged implementation and its related dependency/configuration so AnchoredOverlay/Overlay return to portal-based positioning.
Changes:
- Removed the
primer_react_css_anchor_positioningfeature flag and all related code paths (polyfill loading, data attributes, wrapper/anchor CSS). - Restored
Overlayto always render viaPortaland simplified associated CSS positioning rules. - Cleaned up tests/stories/dependencies to remove the feature-flag-specific behavior and dependency (
@oddbird/css-anchor-positioning).
Reviewed changes
Copilot reviewed 11 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Overlay/Overlay.tsx | Removes CSS anchor positioning feature-flag branching; always portals overlays. |
| packages/react/src/Overlay/Overlay.module.css | Removes anchor-position gating selectors; applies positioning vars unconditionally. |
| packages/react/src/FeatureFlags/DefaultFeatureFlags.ts | Removes the CSS anchor positioning flag from defaults. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx | Removes CSS anchor positioning code path and polyfill behavior. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx | Drops feature-flag parameterized tests and related assertions. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.module.css | Removes CSS anchor positioning classes (Wrapper, Anchor, AnchoredOverlay). |
| packages/react/src/ActionMenu/ActionMenu.tsx | Removes className merging logic in anchor cloning/handler merging paths. |
| packages/react/src/ActionMenu/ActionMenu.test.tsx | Removes feature-flag-based coverage for CSS anchor positioning behavior. |
| packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx | Removes example stories added for CSS anchor positioning demo scenarios. |
| packages/react/package.json | Removes @oddbird/css-anchor-positioning dependency. |
| package-lock.json | Updates lockfile to reflect dependency removal and resulting tree changes. |
| .playwright/snapshots/components/AnchoredOverlay.test.ts-snapshots/css-anchor-positioning | Updates VRT snapshots for the CSS-anchor-positioning variant. |
| .changeset/nine-buttons-lose.md | Removes the changeset associated with the reverted feature. |
You can also share your feedback on Copilot code review. Take the survey.
| top={position?.top || 0} | ||
| left={position?.left || 0} | ||
| responsiveVariant={variant.narrow === 'fullscreen' ? 'fullscreen' : undefined} | ||
| anchorSide={cssAnchorPositioning ? undefined : position?.anchorSide} | ||
| className={clsx(className, overlayClassName, cssAnchorPositioning ? classes.AnchoredOverlay : undefined)} | ||
| anchorSide={position?.anchorSide} | ||
| className={className} | ||
| preventOverflow={preventOverflow} | ||
| data-component="AnchoredOverlay" | ||
| {...restOverlayProps} | ||
| {...overlayProps} | ||
| ref={node => { |
There was a problem hiding this comment.
AnchoredOverlay sets className={className} but then spreads ...overlayProps afterward, so overlayProps.className will override (not merge with) the className prop. This makes it impossible to reliably add a class via className when consumers also provide overlayProps.className (which is used widely in stories). Consider merging the two class names explicitly (and avoid overriding either).
See below for a potential fix:
preventOverflow={preventOverflow}
data-component="AnchoredOverlay"
{...overlayProps}
className={clsx(className, overlayProps?.className)}
| {React.cloneElement(child, { | ||
| ...anchorProps, | ||
| ref: anchorRef, | ||
| className: clsx(anchorProps.className, child.props.className), | ||
| onClick: onButtonClick, | ||
| onKeyDown: onButtonKeyDown, | ||
| })} |
There was a problem hiding this comment.
ActionMenu.Anchor clones its child with ...anchorProps but no longer merges className with child.props.className. If the wrapped element already has a className (common for custom triggers), it will be overwritten by ActionMenu.Anchor's className prop, causing styling regressions. Consider merging the two class names when cloning.
| export const DefaultFeatureFlags = FeatureFlagScope.create({ | ||
| primer_react_breadcrumbs_overflow_menu: false, | ||
| primer_react_css_anchor_positioning: false, | ||
| primer_react_css_has_selector_perf: false, |
There was a problem hiding this comment.
This PR removes the primer_react_css_anchor_positioning flag from defaults, but the Playwright VRT still runs each AnchoredOverlay story twice (with and without that flag) and maintains a second set of *-css-anchor-positioning-* snapshots. If the feature is fully reverted, the VRT parameterization and snapshots should also be removed/updated to avoid redundant coverage and future confusion.
| primer_react_css_has_selector_perf: false, | |
| primer_react_css_has_selector_perf: false, | |
| primer_react_css_anchor_positioning: false, |
| Github | ||
| </ActionList.LinkItem> |
There was a problem hiding this comment.
Brand spelling: "Github" should be "GitHub".
This reverts commit 61ae74c.
Saw some visual regressions during release testing, reverting