Improve mobile modal UX with bottom sheet style#901
Open
dadofsambonzuki wants to merge 1 commit intoteambtcmap:mainfrom
Open
Improve mobile modal UX with bottom sheet style#901dadofsambonzuki wants to merge 1 commit intoteambtcmap:mainfrom
dadofsambonzuki wants to merge 1 commit intoteambtcmap:mainfrom
Conversation
✅ Deploy Preview for btcmap ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Review Summary by QodoRedesign modal with mobile bottom sheet and responsive layout
WalkthroughsDescription• Redesigned modal layout for mobile-first approach with bottom sheet styling • Changed overflow handling from auto to hidden with scrollable content wrapper • Adjusted spacing and padding for mobile devices with responsive breakpoints • Updated breakpoint from md to sm for desktop modal centering behavior Diagramflowchart LR
A["Modal Component"] -->|"Mobile: bottom sheet"| B["Fixed bottom positioning<br/>inset-x-4 bottom-4"]
A -->|"Desktop: centered"| C["Centered modal<br/>sm breakpoint"]
B -->|"Content scrolling"| D["Scrollable wrapper<br/>overflow-y-auto"]
C -->|"Standard styling"| E["Rounded corners<br/>fixed dimensions"]
File Changes1. src/components/Modal.svelte
|
Contributor
Code Review by Qodo
|
📝 WalkthroughWalkthroughThe modal component's layout was restructured from a full-screen sizing approach to a floating, constrained panel positioned near the bottom with maximum height limits. Overflow handling was relocated from the outer container to an inner wrapper, and responsive utility classes were updated with adjusted breakpoints and internal padding adjustments. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Modal.svelte (1)
59-61: Harden scroll container withmin-h-0for flex overflow reliability.At Line 59,
flex-1 overflow-y-autois good, but addingmin-h-0avoids known flexbox overflow edge cases (notably mobile Safari/Firefox) where the slot content may not become scrollable as expected. This is relevant for long modal lists (e.g.,src/components/AppDownloadModal.svelte, Lines 35-50).♻️ Small hardening diff
- <div class="flex-1 overflow-y-auto -mx-5 px-5"> + <div class="min-h-0 flex-1 overflow-y-auto -mx-5 px-5"> <slot /> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modal.svelte` around lines 59 - 61, The scroll container div wrapping the modal slot (the element with classes "flex-1 overflow-y-auto" in src/components/Modal.svelte) can fail to scroll in some flexbox edge cases; add "min-h-0" to that container's class list so the element can properly shrink and allow overflow scrolling (update the div that contains <slot /> to include min-h-0 alongside flex-1 and overflow-y-auto).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Modal.svelte`:
- Line 50: Add a full-viewport backdrop element to enforce true modal behavior:
insert a fixed inset-0 backdrop div rendered when the modal is open (placed
behind the floating panel whose current class has z-[2000]) with a semi-opaque
bg and a lower z-index (e.g., z-[1000]) that captures clicks and calls the
component's existing close handler (the same function used to close the modal on
other interactions); ensure the backdrop prevents pointer events to underlying
content and mark non-modal UI as inert/aria-hidden while the modal is open (set
document.body.inert = true or toggle aria-hidden on the app root when showing
the modal and revert on close) so background controls are not interactable even
with aria-modal="true".
---
Nitpick comments:
In `@src/components/Modal.svelte`:
- Around line 59-61: The scroll container div wrapping the modal slot (the
element with classes "flex-1 overflow-y-auto" in src/components/Modal.svelte)
can fail to scroll in some flexbox edge cases; add "min-h-0" to that container's
class list so the element can properly shrink and allow overflow scrolling
(update the div that contains <slot /> to include min-h-0 alongside flex-1 and
overflow-y-auto).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39c520af-3c9c-4a6e-bf91-ef460f32d32d
📒 Files selected for processing (1)
src/components/Modal.svelte
| aria-modal="true" | ||
| aria-labelledby={titleId} | ||
| class="z-[2000] flex flex-col overflow-auto border border-gray-300 bg-white p-6 shadow-2xl fixed inset-0 w-full h-full dark:border-white/95 dark:bg-dark md:inset-auto md:top-1/2 md:left-1/2 md:w-80 md:max-h-[90vh] md:h-auto md:rounded-xl md:translate-x-[-50%] md:translate-y-[-50%]" | ||
| class="z-[2000] flex flex-col overflow-hidden border border-gray-300 bg-white shadow-2xl fixed inset-x-4 bottom-4 max-h-[85dvh] rounded-2xl dark:border-white/95 dark:bg-dark sm:inset-auto sm:top-1/2 sm:left-1/2 sm:w-80 sm:max-h-[90vh] sm:h-auto sm:rounded-xl sm:translate-x-[-50%] sm:translate-y-[-50%] sm:px-6 sm:py-6 px-5 py-5" |
There was a problem hiding this comment.
Add a viewport backdrop/inert layer for true modal behavior.
At Line 50, the dialog is now a floating panel, but there’s no full-screen backdrop layer. With aria-modal="true", background content should not remain interactable; currently, taps outside the sheet can hit underlying page controls.
💡 Suggested structural fix
{`#if` open}
- <OutClick on:outclick={() => (open = false)}>
+ <div class="fixed inset-0 z-[1999] bg-black/40" aria-hidden="true"></div>
+ <OutClick on:outclick={() => (open = false)}>
<div
bind:this={modalEl}
transition:fly={{ y: 200, duration: 300 }}
role="dialog"
aria-modal="true"
aria-labelledby={titleId}
- class="z-[2000] flex flex-col overflow-hidden border border-gray-300 bg-white shadow-2xl fixed inset-x-4 bottom-4 max-h-[85dvh] rounded-2xl dark:border-white/95 dark:bg-dark sm:inset-auto sm:top-1/2 sm:left-1/2 sm:w-80 sm:max-h-[90vh] sm:h-auto sm:rounded-xl sm:translate-x-[-50%] sm:translate-y-[-50%] sm:px-6 sm:py-6 px-5 py-5"
+ class="z-[2000] fixed inset-x-4 bottom-4 flex flex-col overflow-hidden rounded-2xl border border-gray-300 bg-white shadow-2xl max-h-[85dvh] px-5 py-5 dark:border-white/95 dark:bg-dark sm:inset-auto sm:left-1/2 sm:top-1/2 sm:h-auto sm:w-80 sm:max-h-[90vh] sm:translate-x-[-50%] sm:translate-y-[-50%] sm:rounded-xl sm:px-6 sm:py-6"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Modal.svelte` at line 50, Add a full-viewport backdrop element
to enforce true modal behavior: insert a fixed inset-0 backdrop div rendered
when the modal is open (placed behind the floating panel whose current class has
z-[2000]) with a semi-opaque bg and a lower z-index (e.g., z-[1000]) that
captures clicks and calls the component's existing close handler (the same
function used to close the modal on other interactions); ensure the backdrop
prevents pointer events to underlying content and mark non-modal UI as
inert/aria-hidden while the modal is open (set document.body.inert = true or
toggle aria-hidden on the app root when showing the modal and revert on close)
so background controls are not interactable even with aria-modal="true".
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+59
to
+61
| <div class="flex-1 overflow-y-auto -mx-5 px-5"> | ||
| <slot /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
1. Modal scroll can be clipped 🐞 Bug ≡ Correctness
The new scroll wrapper is a flex child inside a column flex parent with a max-height, but it lacks min-h-0, so it may not shrink and the parent’s overflow-hidden will clip content instead of allowing scrolling. Any modal whose slot content exceeds max-h-[85dvh] can become partially inaccessible.
Agent Prompt
## Issue description
`Modal.svelte` uses a `flex flex-col` container with `max-h-*` and `overflow-hidden`, and delegates scrolling to an inner `flex-1 overflow-y-auto` wrapper. Without `min-h-0`, the inner flex item may not shrink, so it won’t become scrollable and content gets clipped.
## Issue Context
The modal is used for list-based content (language selection, app store links) where the slot content can exceed the sheet height.
## Fix Focus Areas
- src/components/Modal.svelte[50-61]
## Suggested change
Add `min-h-0` to the scroll container (or to an appropriate flex wrapper) so it can shrink and scroll:
- e.g. `class="min-h-0 flex-1 overflow-y-auto ..."`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| aria-modal="true" | ||
| aria-labelledby={titleId} | ||
| class="z-[2000] flex flex-col overflow-auto border border-gray-300 bg-white p-6 shadow-2xl fixed inset-0 w-full h-full dark:border-white/95 dark:bg-dark md:inset-auto md:top-1/2 md:left-1/2 md:w-80 md:max-h-[90vh] md:h-auto md:rounded-xl md:translate-x-[-50%] md:translate-y-[-50%]" | ||
| class="z-[2000] flex flex-col overflow-hidden border border-gray-300 bg-white shadow-2xl fixed inset-x-4 bottom-4 max-h-[85dvh] rounded-2xl dark:border-white/95 dark:bg-dark sm:inset-auto sm:top-1/2 sm:left-1/2 sm:w-80 sm:max-h-[90vh] sm:h-auto sm:rounded-xl sm:translate-x-[-50%] sm:translate-y-[-50%] sm:px-6 sm:py-6 px-5 py-5" |
Contributor
There was a problem hiding this comment.
2. Background click-through actions 🐞 Bug ☼ Reliability
On mobile the modal is no longer full-screen (fixed inset-x-4 bottom-4 ...), but there is no backdrop element to intercept pointer events; taps above the bottom sheet can activate underlying links/buttons and then close the modal via OutClick. This can cause unintended navigation or actions (e.g., footer links behind LanguageModal or app cards behind AppDownloadModal).
Agent Prompt
## Issue description
After the bottom-sheet change, the modal no longer covers the full viewport on mobile, and there is no backdrop layer. Taps outside the sheet therefore hit underlying page elements (links/buttons) and can trigger navigation/actions, then the modal closes.
## Issue Context
This is especially risky where modals are used over interactive UIs (footer nav links; app cards).
## Fix Focus Areas
- src/components/Modal.svelte[42-64]
## Suggested change
Render a full-screen backdrop behind the dialog when `open` is true, and ensure it intercepts pointer events:
- Add a `div` like `class="fixed inset-0"` (optionally with a translucent background)
- Attach `on:click={() => (open = false)}` to the backdrop
- Ensure clicks inside the dialog don’t bubble to the backdrop (`on:click|stopPropagation` on the dialog container, or structure so backdrop and dialog are siblings)
- This also prevents interaction with underlying content while the modal is open.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the shared Modal component to improve mobile usability by shifting to a bottom-sheet layout with a fixed header and a scrollable body, while keeping a centered dialog layout on larger screens.
Changes:
- Switch modal positioning/styling to a mobile bottom-sheet (rounded corners, inset margins,
max-hconstraint). - Prevent the header from scrolling by moving scrolling behavior into an inner content wrapper.
- Adjust padding/spacing rules across breakpoints.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </div> | ||
|
|
||
| <slot /> | ||
| <div class="flex-1 overflow-y-auto -mx-5 px-5"> |
There was a problem hiding this comment.
The scrollable content wrapper is a flex child (flex-1) inside a column flex container with a constrained height (max-h-*). In this layout, overflow-y-auto commonly won’t work correctly unless the flex child is allowed to shrink; elsewhere in the codebase scrollable flex regions include min-h-0. Add min-h-0 (and keep overflow-y-auto) so the slot content reliably scrolls instead of overflowing the modal.
Suggested change
| <div class="flex-1 overflow-y-auto -mx-5 px-5"> | |
| <div class="min-h-0 flex-1 overflow-y-auto -mx-5 px-5"> |
| </div> | ||
|
|
||
| <slot /> | ||
| <div class="flex-1 overflow-y-auto -mx-5 px-5"> |
There was a problem hiding this comment.
The inner wrapper uses -mx-5 px-5 to counteract the modal’s horizontal padding, but the modal padding becomes sm:px-6 on larger screens. Since the inner wrapper doesn’t also switch to 6, the content area will be horizontally misaligned relative to the header on sm+. Make the negative margins/padding responsive as well (e.g., match sm:px-6 with sm:-mx-6 sm:px-6) or remove the negative margins approach.
Suggested change
| <div class="flex-1 overflow-y-auto -mx-5 px-5"> | |
| <div class="flex-1 overflow-y-auto -mx-5 px-5 sm:-mx-6 sm:px-6"> |
Contributor
|
Contributor
|
Why did netlify did not trigger an automatic build for you? |
Member
Author
Prbly because it was a branch from a forked repo. |
Member
Author
Yep. They both transition from bottom for me.
I also see dedicated close X 🤔 I'm Chromium on Android. |
Contributor
I see that now
Same, no transition effect for the android one, weird Wanna let your AI check the comments? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes language and apps modals more mobile-friendly with:\n- Bottom sheet style on mobile (slides up from bottom with rounded corners)\n- Better spacing with padding on mobile\n- Scrollable content area with proper overflow handling\n- Desktop modal remains centered with standard styling
Summary by CodeRabbit