-
Notifications
You must be signed in to change notification settings - Fork 658
Replace polyfill with useAnchoredPosition
#7725
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
Changes from all commits
7d19757
2515e42
7dee64b
8125f08
1841ecc
848c19c
cc8c862
8a5fedf
1162d6c
077bf38
87c58e8
cb72ef8
f49729c
bdaee56
31369df
1388a51
463f087
e927b42
5a0e340
e7989c0
c88c994
621ccab
6d98342
9763d3b
5e712d0
b92d59f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@primer/react': minor | ||
| --- | ||
|
|
||
| AnchoredOverlay: Remove polyfill for CSS Anchor Positioning, use primer/behaviors as fallback. Ensure overlays take available space. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,31 +27,56 @@ | |
| margin: 0; | ||
| padding: 0; | ||
| border: 0; | ||
| max-height: none; | ||
| max-width: none; | ||
| } | ||
|
|
||
| &[data-side='outside-bottom'] { | ||
| /* stylelint-disable primer/spacing */ | ||
| top: calc(anchor(bottom) + var(--base-size-4)); | ||
| left: anchor(left); | ||
|
|
||
| &[data-align='left'] { | ||
| left: auto; | ||
| right: calc(anchor(right) - var(--anchored-overlay-anchor-offset-left)); | ||
| } | ||
| } | ||
|
|
||
| &[data-side='outside-top'] { | ||
| margin-bottom: var(--base-size-4); | ||
| bottom: anchor(top); | ||
| left: anchor(left); | ||
|
|
||
| &[data-align='left'] { | ||
| left: auto; | ||
| right: anchor(right); | ||
| } | ||
| } | ||
|
|
||
| &[data-side='outside-left'] { | ||
| right: anchor(left); | ||
| top: anchor(top); | ||
| margin-right: var(--base-size-4); | ||
| position-try-fallbacks: flip-inline, flip-block, flip-start, --outside-left-to-bottom; | ||
| } | ||
|
|
||
| &[data-side='outside-right'] { | ||
| left: anchor(right); | ||
| top: anchor(top); | ||
| margin-left: var(--base-size-4); | ||
| position-try-fallbacks: flip-inline, flip-block, flip-start, --outside-right-to-bottom; | ||
| } | ||
| } | ||
|
|
||
| @position-try --outside-left-to-bottom { | ||
| right: anchor(right); | ||
| top: calc(anchor(bottom) + var(--base-size-4)); | ||
| margin: 0; | ||
| width: auto; | ||
| } | ||
|
|
||
| @position-try --outside-right-to-bottom { | ||
| left: anchor(left); | ||
| top: calc(anchor(bottom) + var(--base-size-4)); | ||
| margin: 0; | ||
| width: auto; | ||
| } | ||
|
Comment on lines
+70
to
+82
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding some more fallbacks for |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,7 @@ import {XIcon} from '@primer/octicons-react' | |||||||||||||||||
| import classes from './AnchoredOverlay.module.css' | ||||||||||||||||||
| import {clsx} from 'clsx' | ||||||||||||||||||
| import {useFeatureFlag} from '../FeatureFlags' | ||||||||||||||||||
| import {widthMap} from '../Overlay/Overlay' | ||||||||||||||||||
|
|
||||||||||||||||||
| interface AnchoredOverlayPropsWithAnchor { | ||||||||||||||||||
| /** | ||||||||||||||||||
|
|
@@ -125,17 +126,6 @@ export type AnchoredOverlayProps = AnchoredOverlayBaseProps & | |||||||||||||||||
| (AnchoredOverlayPropsWithAnchor | AnchoredOverlayPropsWithoutAnchor) & | ||||||||||||||||||
| Partial<Pick<PositionSettings, 'align' | 'side' | 'anchorOffset' | 'alignmentOffset' | 'displayInViewport'>> | ||||||||||||||||||
|
|
||||||||||||||||||
| const applyAnchorPositioningPolyfill = async () => { | ||||||||||||||||||
| if (typeof window !== 'undefined' && !('anchorName' in document.documentElement.style)) { | ||||||||||||||||||
| try { | ||||||||||||||||||
| await import('@oddbird/css-anchor-positioning') | ||||||||||||||||||
| } catch (e) { | ||||||||||||||||||
| // eslint-disable-next-line no-console | ||||||||||||||||||
| console.warn('Failed to load CSS anchor positioning polyfill:', e) | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| const defaultVariant = { | ||||||||||||||||||
| regular: 'anchored', | ||||||||||||||||||
| narrow: 'anchored', | ||||||||||||||||||
|
|
@@ -173,7 +163,9 @@ export const AnchoredOverlay: React.FC<React.PropsWithChildren<AnchoredOverlayPr | |||||||||||||||||
| displayCloseButton = true, | ||||||||||||||||||
| closeButtonProps = defaultCloseButtonProps, | ||||||||||||||||||
| }) => { | ||||||||||||||||||
| const cssAnchorPositioning = useFeatureFlag('primer_react_css_anchor_positioning') | ||||||||||||||||||
| const cssAnchorPositioningFlag = useFeatureFlag('primer_react_css_anchor_positioning') | ||||||||||||||||||
| const supportsNativeCSSAnchorPositioning = useRef(false) | ||||||||||||||||||
|
||||||||||||||||||
| const supportsNativeCSSAnchorPositioning = useRef(false) | |
| const supportsNativeCSSAnchorPositioning = useRef( | |
| typeof document !== 'undefined' && | |
| typeof window !== 'undefined' && | |
| typeof window.CSS !== 'undefined' && | |
| window.CSS.supports('anchor-name: --primer-overlay-anchor') && | |
| window.CSS.supports('position-anchor: --primer-overlay-anchor'), | |
| ) |
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.
I took a second to understand these 😅
Is leftOffset from ensuring overlayLeft >= viewportMargin, so viewportMargin - (rect.right - overlayWidth)>=0
and rightOffset from overlayRight <= vw - viewportMargin?
Could we add a short comment here explaining the formulas?
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.
You're right, it can be a bit confusing 😅 I added a comment to clarify! Thanks for the suggestion too!
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.
We calculate the amount of space available surrounding the anchor (x-axis). If there's not enough space for the overlay in the viewport (e.g., viewport width is 600px while the overlay width is 640px), we'll add an offset.
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.
Odd?
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.
I'm thinking that the added is the correct version. We used to override
max-height, which meant that content would just expand the full height of the content. Now we respect themax-heightset.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.
are there similar instances in github-ui that we need to verify?
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.
Yeah! This issue is what made me make the change: https://github.com/github/primer/issues/6538. It uses
max-height