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(ui): modal scrolling behavior on mobile #1620

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

unrenamed
Copy link
Contributor

@unrenamed unrenamed commented Oct 23, 2024

What does this PR do?

Moves the scrollable section one level down, placing it directly inside <Drawer.Content /> as its child. This change ensures that when scrolling is active, no :after pseudo-elements are unintentionally visible. Improves visual consistency and resolves minor aesthetic bugs related to scrollbars and overflow.

Fixes: #1609

TL;DR

Although @aritradevelops were right in their #1612 PR about the use of Vaul's drawer that is shipped with some default CSS, the actual root cause was quite unexpected.

Screenshot 2024-10-23 at 18 01 35

Issue Diagnosis

Who is the bad guy?

This was the first question to answer: how is it possible that the scrollable drawer from the official Vaul website does NOT have such an issue. Check this yourself: https://vaul.emilkowal.ski/default#scrollable. It also has that same :after pseudo-element with height: 200% property.

Screenshot 2024-10-23 at 18 32 34

And if you take a closer look, you may notice that the <Drawer.Content /> isn’t the one getting scrolled when its content expands. Nope, it’s the first child <div> block that’s hogging all the action!

Screenshot 2024-10-23 at 18 34 35

But why does it break the UI?

It's easy. overflow: auto enables scrolling for the drawer parent container (the <Drawer.Content /> component). The default CSS shipped with the package contains an :after pseudo-element, which is a child of the parent container. Enabling scrolling for the parent container made all children — including the :after — visible. Removing overflow: auto fixes the problem, but introduces another: the Create button gets cut off at the bottom.

Ugh, what now?

Do you see that max-h-[95h] class on the parent element? It does a few things:

  1. restricts the height of an element to a maximum value, in this case, 95% of the viewport height
  2. the element grows up to 95vh, but not beyond it, even if its content exceeds this height
  3. when content inside the element is taller than 95vh, the element will either overflow (if overflow is allowed) or be constrained by its height.

Key point: if overflow is allowed. And guess what? It’s not! We removed it from the parent container, remember? Did we add one to any of its children? Nope!

So, what's the fix?

Adding overflow or overflow-y to the children of the <Drawer.Content /> allows them to handle scrolling when the inner content exceeds the specified height. Check the code for a clearer picture.

Why this is anyway better than the fix suggested in #1612 PR?

I see two big red flags with overriding CSS globally:

  1. Overuse of !important: It makes future overrides difficult and leads to maintenance issues.
  2. Global impact: The rule applies broadly to all matching elements, potentially causing unintended side effects across the app. E.g. in other component using vaul's drawer like input select or popover.

Result

This is how the fixed modal looks on mobile:

Screen.Recording.2024-10-23.at.18.08.41.mov

And this is how it looks in full screen:

Screen.Recording.2024-10-23.at.18.11.07.mov

Moves the scrollable section one level down, placing it directly inside <Drawer.Content /> as its child. This change ensures that when scrolling is active, no :after pseudo-elements are unintentionally visible. Improves visual consistency and resolves minor aesthetic bugs related to scrollbars and overflow.
Copy link

vercel bot commented Oct 23, 2024

@unrenamed is attempting to deploy a commit to the Dub Team on Vercel.

A member of the Team first needs to authorize it.

@unrenamed
Copy link
Contributor Author

@steven-tey Just merged with the latest main. Feel free to review the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI Breaks on Add New API Key Drawer with Restricted Permissions on Mobile
1 participant