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

fix: Menu overflows #340

Merged
merged 3 commits into from
Sep 22, 2023
Merged

fix: Menu overflows #340

merged 3 commits into from
Sep 22, 2023

Conversation

matthewlipski
Copy link
Collaborator

@matthewlipski matthewlipski commented Sep 13, 2023

While we handle overflows for Tippys and Mantine Menu.Dropdown components to some extent (i.e. flipping the position when there's no space), for very small viewports where there's no space an any available position, the element goes beyond the viewport. Unfortunately, this is not something easily fixed with the use of Menu props, so this PR adds a new hook which dynamically adjusts maxHeight for Menu.Dropdown components when they're rendered. This hooks is used by almost all UI elements which use a Menu (slash menu, color picker, and formatting toolbar dropdown).

Closes #339

@vercel
Copy link

vercel bot commented Sep 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
blocknote-website ✅ Ready (Inspect) Visit Preview Sep 22, 2023 4:21pm

Copy link
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewlipski, can't we set a max-height to for example 100vh using CSS? won't that solve the problem?

Also, in Floating UI I think this becomes easier: https://floating-ui.com/docs/size

@matthewlipski
Copy link
Collaborator Author

@matthewlipski, can't we set a max-height to for example 100vh using CSS? won't that solve the problem?

Also, in Floating UI I think this becomes easier: https://floating-ui.com/docs/size

Not really, since this would make the dropdown the same height as the viewport, but in most cases the top of the dropdown is not in the same place as the top of the viewport, so it would still overflow.

@dukesx
Copy link

dukesx commented Sep 19, 2023

How about also adding a constrained Dropdown height and let the inside elements as overflow scroll?

@YousefED
Copy link
Collaborator

@matthewlipski, can't we set a max-height to for example 100vh using CSS? won't that solve the problem?
Also, in Floating UI I think this becomes easier: https://floating-ui.com/docs/size

Not really, since this would make the dropdown the same height as the viewport, but in most cases the top of the dropdown is not in the same place as the top of the viewport, so it would still overflow.

Got it. Is the solution at the bottom of this page https://mantine.dev/core/scroll-area/ possible with menus? If so, that would be more of a Mantine-style fix I think

How about also adding a constrained Dropdown height and let the inside elements as overflow scroll?

@dukesx you mean a fixed height?

@dukesx
Copy link

dukesx commented Sep 20, 2023

@YousefED yes exactly. I figure that trying to make it responsive through javascript is just a bottleneck.

@matthewlipski
Copy link
Collaborator Author

@matthewlipski, can't we set a max-height to for example 100vh using CSS? won't that solve the problem?
Also, in Floating UI I think this becomes easier: https://floating-ui.com/docs/size

Not really, since this would make the dropdown the same height as the viewport, but in most cases the top of the dropdown is not in the same place as the top of the viewport, so it would still overflow.

Got it. Is the solution at the bottom of this page https://mantine.dev/core/scroll-area/ possible with menus? If so, that would be more of a Mantine-style fix I think

How about also adding a constrained Dropdown height and let the inside elements as overflow scroll?

@dukesx you mean a fixed height?

I played around with Scrollarea.Autosize but unfortunately it doesn't seem to be able to replace the JS logic :/ It will make the element inside it scrollable once it goes beyond a predetermined max height, but we're still left with the issue of calculating that max height (which is currently done using the custom hook). Using 100vh still won't work as the dropdown doesn't always span from the very top to the very bottom of the viewport, depending on the selected block's position.

Also yeah this seems really easy to do with FloatingUI, it's frustrating that Mantine heavily limits the middleware customization (can only set { flip: boolean, shift: boolean, inline: boolean }), even though it seems to use FloatingUI itself.

@YousefED
Copy link
Collaborator

Would be nice if we can get rid of the setTimeout somehow. For now, let's add a comment why this "hack" is necessary

Looking at the source code, it's possible that in the latest mantine (v7) you can pass a ref to menu.dropdown but for now this PR is ok imo

@matthewlipski matthewlipski merged commit 9a10c68 into main Sep 22, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdowns don't have overflow-y scroll
3 participants