Replace Radix UI dialog overlay with custom overlay to fix layout shift in VSCode webview#24
Conversation
…ft in VSCode webview - Replace DialogPrimitive.Overlay with simple div using absolute positioning - Add manual scroll lock with body.dialog-open class - Fix background padding changes when dialog opens in VSCode environment issue #23
Summary of ChangesHello @kubrickcode, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to resolve an annoying layout shift issue experienced in the VSCode webview when dialogs are opened. The solution involves replacing the default Radix UI dialog overlay with a custom-built one, which allows for precise control over its positioning and behavior. Additionally, a manual scroll lock mechanism has been introduced to prevent background content from shifting or scrolling when a dialog is active, ensuring a smoother user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the Radix UI dialog overlay with a custom implementation to fix a layout shift issue in VSCode webviews. The changes include a new CustomOverlay React component that manually manages body scroll-locking via a CSS class, and a corresponding CSS rule. My review focuses on improving the robustness of the scroll-locking mechanism to handle multiple dialogs and on CSS best practices. Overall, this is a good solution to the problem described.
There was a problem hiding this comment.
Code Review
This pull request replaces the Radix UI dialog overlay with a custom implementation to resolve a layout shift issue within VSCode webviews. The changes involve creating a new CustomOverlay component that manually manages a dialog-open class on the <body> to lock scrolling, and updating the CSS to apply overflow: hidden. The approach is sound, but I have a couple of suggestions to improve the robustness and maintainability of the implementation.
| React.useEffect(() => { | ||
| document.body.classList.add('dialog-open'); | ||
|
|
||
| return () => { | ||
| document.body.classList.remove('dialog-open'); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
The current implementation for scroll locking by toggling the dialog-open class on document.body is not robust against multiple dialogs being open concurrently. If two dialogs are open and one is closed, it will remove the dialog-open class, re-enabling scrolling while the second dialog is still open.
To fix this, you would need to track the number of active dialogs. A common approach is to use a module-level counter that is incremented when a dialog mounts and decremented when it unmounts. The class on <body> would only be added when the counter goes from 0 to 1, and removed when it goes from 1 to 0.
If the application design guarantees that only one dialog can be open at any time, the current approach is acceptable, but it would be good practice to add a comment to the useEffect hook to clarify this assumption for future maintainability.
| body.dialog-open { | ||
| overflow: hidden !important; | ||
| } |
There was a problem hiding this comment.
Using !important is generally discouraged as it can make styles harder to manage and override later, because it breaks the natural CSS cascade. While it might be necessary in this case to override styles injected by the VSCode webview environment, it's important to document why it's being used. Please add a comment explaining the necessity of !important here.
body.dialog-open {
/* Using !important is necessary to override VSCode webview styles and prevent layout shift. */
overflow: hidden !important;
}1 participant
issue #23