bugfix(Dialog): Fixing issue where page shifts when dialog is open #6266
Conversation
github-actions
bot
added
staff
|
👋 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! |
size-limit report 📦
|
🦋 Changeset detectedLatest commit: 8d35de2 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 |
jonrohan
changed the title
| return () => { | ||
| document.body.style.overflow = bodyOverflowStyle | ||
| } | ||
| document.body.style.setProperty('--dialog-scrollgutter', `${window.innerWidth - document.body.clientWidth}px`) |
There was a problem hiding this comment.
I cleared up this overflow style setting since we can include it in the class. The property I'm setting here is identical to what we have in PVC https://github.com/primer/view_components/blob/2677324d35cac1ce5ebf9aede851fa4267f7c6e2/app/components/primer/dialog_helper.ts#L81
But I want to know if useEffect is the correct callback to have this in?
There was a problem hiding this comment.
@jonrohan I think that's the right place to have it 👍
| @@ -339,7 +327,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP | |||
| data-width={width} | |||
| data-height={height} | |||
| sx={sx} | |||
| className={clsx(className, classes.Dialog)} | |||
| className={clsx(className, classes.Dialog, classes.DisableScroll)} | |||
There was a problem hiding this comment.
Opted to add a separate class here in case we wanted to make this a prop in the future, which is what we do in PVC
| inherits: false; | ||
| syntax: '<length>'; | ||
| } | ||
|
|
There was a problem hiding this comment.
Most of this implementation was taken from what we did in PVC primer/view_components#2542
| return () => { | ||
| document.body.style.overflow = bodyOverflowStyle | ||
| } | ||
| document.body.style.setProperty('--dialog-scrollgutter', `${window.innerWidth - document.body.clientWidth}px`) |
There was a problem hiding this comment.
@jonrohan I think that's the right place to have it 👍
Closes https://github.com/github/primer/issues/5376
This PR changes how we turn on disable scrolling when the dialog is open. The strategy was taken from what we do in PVC. https://github.com/primer/view_components/blob/2677324d35cac1ce5ebf9aede851fa4267f7c6e2/app/components/primer/dialog_helper.ts#L81
We essentially set a variable on the body element that equals the width of the scrollbar. And turns padding on when that scrollbar is hidden.
I also used this as an opportunity to cleanup the old inline styles we were doing for hidding, and migrated the test to vitest in the process for the toHaveStyle assertion.
Changelog
New
Changed
Dialog adds padding to page when scrollbar is hidden.
Removed
Rollout strategy
Testing & Reviewing
Merge checklist