-
Notifications
You must be signed in to change notification settings - Fork 507
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
[Dialog] Add responsive position
props
#4214
Conversation
🦋 Changeset detectedLatest commit: c1b4595 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 |
size-limit report 📦
|
Co-authored-by: Mike Perrotti <mperrotti@users.noreply.github.com>
Discovered a problem where if you set the regular position to Video broken.mov
⬆️ This part was confusing due to the Storybook props. I suggest we align them directly with the component props for clarity. To demonstrate how responsive props function, we could include extra Storybook code examples that focus on responsiveness without involving props. I've done that in my pull request. |
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.
LGTM! Just left a couple of non-blocking comments/questions
src/Dialog/Dialog.tsx
Outdated
@keyframes Overlay--motion-slideUp { | ||
from { | ||
transform: translateY(100%); | ||
} | ||
} | ||
|
||
@keyframes Overlay--motion-slideInRight { | ||
from { | ||
transform: translateX(-100%); | ||
} | ||
} | ||
|
||
@keyframes Overlay--motion-slideInLeft { | ||
from { | ||
transform: translateX(100%); | ||
} | ||
} |
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.
Random question, when do we use @keyframes
in the styled component itself versus using the keyframes
helper? I honestly wasn't sure and was curious if you knew 🤔
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 not really following your question 🤔 What's the keyframes helper? I just followed what was already here and added the new keyframes, but if there's a better way let me know.
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.
@langermank there's a keyframes
export from styled-components
that is used to define @keyframes
(from what I understand). So these would look like:
const slideInLeft = keyframes`
from {
transform: translateX(100%);
}
`
And then used like animation-name: ${slideInLeft}
.
I just wasn't sure when/where we use them and was curious what the rule was 👀 Totally makes sense that you're adding onto what exists, I'll add this question to the working group session this week to see if it matters at all and/or if there is a preference we have to keep things consistent (or make things easier to translate in the future) 👍
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.
Cool, sounds good! I would think the way I wrote it would be easier to translate to a CSS file for CSS Modules.
src/Dialog/Dialog.tsx
Outdated
&[data-position-regular='center'] { | ||
border-radius: var(--borderRadius-large, 0.75rem); | ||
animation-name: Overlay--motion-scaleFade; | ||
} |
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.
Just curious, is this coming from the current implementation in PVC? 👀 I know you said we had this implemented already there and was curious how portable it felt, if so
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, literally copy/paste. You can go see it in PVC 😄
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.
Super cool! 🔥
src/Dialog/Dialog.tsx
Outdated
@@ -265,6 +359,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP | |||
width = 'xlarge', | |||
height = 'auto', | |||
footerButtons = [], | |||
position = {narrow: 'center', regular: 'center'}, |
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.
Could we move this default value to an object outside of the component? e.g.
const defaultPosition = {
narrow: 'center',
regular: 'center',
};
// ...
const _Dialog = React.forwardRef((props) => {
const { position = defaultPosition } = props
});
This way we don't have to think about the object changing every render later on in the component (which thankfully we don't have to think about this with sx
because of how you approached this 🙌)
Co-authored-by: Josh Black <joshblack@github.com>
This is a riff on Maxime's PR to add bottom sheet support to Primer React. The PVC Dialog is lacking several features that are already part of the Dialog API in PVC, bottom sheet being one of them. The original API uses a
position
prop to handle this, allowing the consumer to set the position for both narrow and regular viewports (or a single position for both). Check out Lookbook for more details.It's easier for our consumers to switch from Rails to React if the component APIs feel similar, so I'm extending
position
to React. This includes:We use left/right for the global nav, and we've seen side sheets used in React apps just not using our Dialog. If we don't want to support that right now I'm fine removing it from this PR.
Responsiveness
This is simply relying on media queries to enforce certain options for narrow only, like
fullscreen
andbottom
.CleanShot.2024-02-02.at.12.37.46.mp4
Changelog
New
position
prop that takes either a stringposition="center"
or a responsive value like `position={{narrow: "fullscreen", regular: "center"}}Changed
Rollout strategy
Testing & Reviewing
Merge checklist