-
Notifications
You must be signed in to change notification settings - Fork 645
fix(AnchoredOverlay): Remove use of useResponsiveValue #7098
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?
Conversation
|
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
| top={position?.top || 0} | ||
| left={position?.left || 0} | ||
| responsiveVariant={variant.narrow === 'fullscreen' ? 'fullscreen' : undefined} | ||
| data-variant={currentResponsiveVariant} |
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.
from what I can tell there is not real usage to this
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
This pull request refactors the Overlay component to use CSS custom properties (CSS variables) for positioning instead of inline styles. The changes move position values (top, left, right, bottom) from direct inline style properties to CSS custom properties that are consumed by the CSS module.
Key changes:
- CSS custom properties (
--top,--left,--right,--bottom) are now set in the inline style instead of direct CSS properties - The CSS module consumes these custom properties to apply positioning
- Removed unused
useResponsiveValueimport and logic fromAnchoredOverlay - Removed
data-variantattribute from the overlay element
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/Overlay/Overlay.tsx | Changed positioning from inline CSS properties to CSS custom properties |
| packages/react/src/Overlay/Overlay.module.css | Added CSS rules to consume the custom properties for positioning |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx | Removed unused responsive variant logic and simplified position prop passing |
| packages/react/src/AnchoredOverlay/snapshots/AnchoredOverlay.test.tsx.snap | Updated snapshot to reflect new CSS custom property style format |
| top={currentResponsiveVariant === 'anchored' ? position?.top || 0 : undefined} | ||
| left={currentResponsiveVariant === 'anchored' ? position?.left || 0 : undefined} |
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 css rule already takes care of resetting top/left, switched up the inline styles in the BaseOverlay as well to be css variables applied by the className so that they don't win over specificity
Co-authored-by: Copilot <[email protected]>
…act into chore/anchored-overlay-ssr-fix
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/5918 |
|
🟢 ci completed with status |
|
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
|
Waiting to merge until landing dotcom changes https://github.com/github/github-ui/pull/6032 |
Closes https://github.com/github/primer/issues/6022
Changelog
Removed
useResponsiveValueinAnchoredOverlayin favor of css solutionRollout strategy
Testing & Reviewing
Merge checklist