Skip to content
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

[experiments] Fully featured Menu + common infra #1330

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Jan 13, 2025

@michaldudak michaldudak added the component: menu This is the name of the generic UI component, not the React module! label Jan 13, 2025
Copy link

netlify bot commented Jan 13, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 8e1df4f
🔍 Latest deploy log https://app.netlify.com/sites/base-ui/deploys/6792469f7398290008ff6d18
😎 Deploy Preview https://deploy-preview-1330--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@atomiks
Copy link
Contributor

atomiks commented Jan 14, 2025

When closing the menu shifts up and to the left by ~5-10px, causing a jitter?

@michaldudak
Copy link
Member Author

michaldudak commented Jan 22, 2025

When closing the menu shifts up and to the left by ~5-10px, causing a jitter?

This is actually an effect of another issue: when useScrollLock is applied, it changes the body styles setting position: relative and box-sizing: border-box. The experiment page uses default styles for the body, so when the scroll lock is removed, the reference for the menu's positioner changes, causing the shift. This might be less of an issue in real applications, as most often, default margin/padding will be removed from the html and body elements (@colmtuite, correct me if I'm wrong).

I'll style the body element to fix the issue here.

For the record - this is how it currently looks:

Screen.Recording.2025-01-22.at.16.15.10.mov

@atomiks
Copy link
Contributor

atomiks commented Jan 23, 2025

This is actually an effect of another issue: when useScrollLock is applied

In order to fix #1343 on the body easily, we need to use mounted not open state to apply the scroll lock. That would also prevent this from being visible?

@michaldudak
Copy link
Member Author

It would prevent this exact problem, but all the other absolutely positioned elements on the page that only have statically positioned parents will be affected (they will shift when a popup opens and after it disappears), as in https://codesandbox.io/p/sandbox/headless-cloud-8s744f?file=%2Fsrc%2FApp.tsx%3A11%2C65.

@michaldudak
Copy link
Member Author

michaldudak commented Jan 23, 2025

One option to prevent this from happening would be to teach developers to set up a positioning context on the .Root element (add position: relative to styles.css in https://base-ui.com/react/overview/quick-start#set-up-portals). We can set position: relative on default portal containers ourselves.

@michaldudak michaldudak marked this pull request as ready for review January 23, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants