-
Notifications
You must be signed in to change notification settings - Fork 603
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
SelectPanel: Add variant=modal #5817
Changes from all commits
fbd625c
1b75223
cc94b85
be1a38a
9e3f6de
ddd5d7d
1f73c86
0cd98d7
93ab700
0cd4c67
def876a
9d48d0a
ab31fb9
0169cf0
15f98a3
6e34ef4
054f0eb
1a336ed
d9640e3
2191153
6e3c5bf
3c54302
174401f
583dd69
b4928a3
e906b5b
a76b712
973ebab
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 | ||
--- | ||
|
||
SelectPanel: Add variant="modal" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,13 +103,15 @@ const StyledOverlay = toggleStyledComponent( | |
max-width: calc(100vw - 2rem); | ||
} | ||
|
||
&:where([data-variant='fullscreen']) { | ||
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. Note for reviewer: Overlay's css module is in GA, so we don't need this css in sx anymore 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. I think let's keep this for now just in case, in theory the ga FF could still be turned off at any moment? The team can remove this when they remove the FF entirely from this component 🙏 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. I thought we weren't adding adding css for new features? Based on #5761 (comment) I recently added this css in #5759, so thought I'll clean it up as well. yay/nay? 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. yeah I think the issue here is the styles won't be added at all because if FF is not enabled the Overlay classname will not be added so these styles will never make it in (why you need that FF on that other test), the correct way of adding new features without sx styling is to always have it go to css modules, for example if you created a separate class and added it to both (enabled, not enabled) implementations |
||
top: 0; | ||
left: 0; | ||
width: 100vw; | ||
height: 100vh; | ||
margin: 0; | ||
border-radius: unset; | ||
&:where([data-responsive='fullscreen']) { | ||
@media screen and (max-width: calc(768px - 0.02px)) { | ||
top: 0; | ||
left: 0; | ||
width: 100vw; | ||
height: 100vh; | ||
margin: 0; | ||
border-radius: unset; | ||
} | ||
} | ||
|
||
${sx}; | ||
|
@@ -127,6 +129,7 @@ type BaseOverlayProps = { | |
role?: AriaRole | ||
children?: React.ReactNode | ||
className?: string | ||
responsiveVariant?: 'fullscreen' // we only support fullscreen today but we might add bottomsheet in the future | ||
} | ||
|
||
type OwnOverlayProps = Merge<StyledOverlayProps, BaseOverlayProps> | ||
|
@@ -265,6 +268,7 @@ const Overlay = React.forwardRef<HTMLDivElement, internalOverlayProps>( | |
role = 'none', | ||
visibility = 'visible', | ||
width = 'auto', | ||
responsiveVariant, | ||
...props | ||
}, | ||
forwardedRef, | ||
|
@@ -322,6 +326,7 @@ const Overlay = React.forwardRef<HTMLDivElement, internalOverlayProps>( | |
right={right} | ||
height={height} | ||
visibility={visibility} | ||
data-responsive={responsiveVariant} | ||
{...props} | ||
/> | ||
</Portal> | ||
|
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.
Note for reviewer: I changed full screen to only be a responsive thing instead of variant you can apply whenever.
data-variant
→data-responsive