Mobile: adjust modals height to visual viewport #4427
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
andrew--r
force-pushed
the
master
branch
2 times, most recently
from
a4f524c to
ccbad73
Compare
andrew--r
changed the title
andrew--r
changed the title
WalkthroughThis pull request implements changes to the modal components within the desktop client to enhance layout and responsiveness. In the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
packages/desktop-client/src/components/modals/PayeeAutocompleteModal.tsx (1)
32-34:⚠️ Potential issueUpdate defaultAutocompleteProps to use visual viewport height.
The height in defaultAutocompleteProps still uses vh units which may cause layout issues with mobile keyboards. Update it to use the visual viewport height for consistency.
const defaultAutocompleteProps = { - containerProps: { style: { height: isNarrowWidth ? '90vh' : 275 } }, + containerProps: { style: { height: isNarrowWidth ? 'calc(var(--visual-viewport-height) * 0.9)' : 275 } }, };packages/desktop-client/src/components/modals/AccountAutocompleteModal.tsx (1)
28-30:⚠️ Potential issueUpdate defaultAutocompleteProps to use visual viewport height.
Similar to PayeeAutocompleteModal, the height in defaultAutocompleteProps should use the visual viewport height for consistent behavior with mobile keyboards.
const defaultAutocompleteProps = { - containerProps: { style: { height: isNarrowWidth ? '90vh' : 275 } }, + containerProps: { style: { height: isNarrowWidth ? 'calc(var(--visual-viewport-height) * 0.9)' : 275 } }, };packages/desktop-client/src/components/modals/CategoryAutocompleteModal.tsx (1)
34-36:⚠️ Potential issueUpdate defaultAutocompleteProps to use visual viewport height.
For consistency with other modal components and proper mobile keyboard handling, update the height calculation to use the visual viewport height.
const defaultAutocompleteProps = { - containerProps: { style: { height: isNarrowWidth ? '90vh' : 275 } }, + containerProps: { style: { height: isNarrowWidth ? 'calc(var(--visual-viewport-height) * 0.9)' : 275 } }, };
🧹 Nitpick comments (2)
packages/desktop-client/src/components/common/Modal.tsx (1)
118-142: Clean up commented code and align viewport unitsTwo suggestions for improvement:
- Remove the commented-out border style on line 131 to maintain clean code.
- Consider using
calc(var(--visual-viewport-height) * 0.9)formaxHeightinstead of90vhto maintain consistency with the visual viewport approach.style={{ flex: 1, padding: 10, willChange: 'opacity, transform', maxWidth: '90vw', minWidth: '90vw', maxHeight: '90vh', - //border: '1px solid ' + theme.modalBorder, color: theme.pageText, backgroundColor: theme.modalBackground,packages/desktop-client/src/components/modals/EditFieldModal.tsx (1)
1-1: Consider centralizing modal height calculations.The height calculations and viewport ratios (0.85, 0.9) are duplicated across multiple modal components. Consider extracting these into a shared utility or constants file to improve maintainability and ensure consistency.
Example approach:
// modalUtils.ts export const MODAL_VIEWPORT_RATIOS = { DEFAULT: 0.85, AUTOCOMPLETE: 0.9 } as const; export const getModalHeight = (isNarrowWidth: boolean, ratio = MODAL_VIEWPORT_RATIOS.DEFAULT) => isNarrowWidth ? `calc(var(--visual-viewport-height) * ${ratio})` : 275;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4427.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/common/Modal.tsx(2 hunks)packages/desktop-client/src/components/modals/AccountAutocompleteModal.tsx(1 hunks)packages/desktop-client/src/components/modals/CategoryAutocompleteModal.tsx(1 hunks)packages/desktop-client/src/components/modals/EditFieldModal.tsx(1 hunks)packages/desktop-client/src/components/modals/PayeeAutocompleteModal.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Functional
- GitHub Check: Visual regression
🔇 Additional comments (4)
packages/desktop-client/src/components/common/Modal.tsx (3)
99-108: Well-implemented solution for mobile viewport handling!The new positioning container effectively addresses the mobile keyboard overlap issue by:
- Using
var(--visual-viewport-height)to respect the keyboard space- Implementing flex layout for proper centering
- Maintaining scrollability with
overflowY: 'auto'
314-323: Good addition of flex: 'none' to header!This change ensures the header maintains a consistent size and doesn't flex unexpectedly within the modal layout.
76-173: Verify modal behavior on Android devicesAs mentioned in the PR objectives, the changes haven't been tested on Android devices. While the implementation looks solid, it would be beneficial to verify:
- Keyboard interaction with modals
- Visual viewport height calculation
- Scrolling behavior
Would you like me to help create a testing checklist for Android devices?
packages/desktop-client/src/components/modals/EditFieldModal.tsx (1)
247-249: LGTM!The height calculation using visual viewport is correctly implemented and consistent with other modal components.
| @@ -311,6 +318,7 @@ export function ModalHeader({ | |||
| alignItems: 'center', | |||
| position: 'relative', | |||
| height: 60, | |||
| flex: 'none', | |||
There was a problem hiding this comment.
This change is intentional: without it the ModalHeader shrinks, which looks bad on small viewports
|
This is definitely a big improvement, thank you! Is there any way to minimise/animate the jump when it closes? There are a couple of times that the popover grows again when the keyboard closes and it takes a bit of time and jumps back. |
|
Fixes #3939 |
|
/update-vrt |
@matt-fidd that's a good point, I've added a height transition to the container, so at least the change is smooth, but I don't think there's a way to get rid of the delay, would be happy to hear any ideas. Here is how it looks like with the transition: ScreenRecording_02-21-2025.23-34-34_1.MP4
Nice, didn't know there is an issue for the problem. Thanks for a quick review! 🙂 |
| willChange: 'opacity, transform', | ||
| maxWidth: '90vw', | ||
| minWidth: '90vw', | ||
| maxHeight: 'var(--visual-viewport-height)', |
There was a problem hiding this comment.
This makes the taller modals (CSV import, see VRT update commit) look a bit odd on desktop, could you limit it to expanding on mobile only?
| maxHeight: 'var(--visual-viewport-height)', | |
| maxHeight: isNarrowWidth ? 'var(--visual-viewport-height)' : '90vh', |
There was a problem hiding this comment.
Ah yes, missed that, changed it to calc(var(--visual-viewport-height) * 0.9) which should be effectively the same (90% of the viewport height): b56ffa7
actual-github-bot
bot
added
⚠️ Changes requested
🔍 Ready for review
and removed
🔍 Ready for review
⚠️ Changes requested
labels
|
/update-vrt |
|
Looks good! Could you add a dummy commit to retrigger the CI, it's a limitation of the updatevrt command at the moment.
|
Motivation
Currently, keyboard on mobile (specifically iOS) overlaps modal's content, which is not great in some cases:
BEFORE.MP4
Solution
React Aria exposes a CSS custom property with a height of the visual viewport:
I slightly changed the layout of the
<Modal>. Now there are two base layers:ReactAriaModalOverlaythat expands to the whole viewport (not the visual one) and provides a backdropOverlay and positioning container should be separate to keep the overlay stable when keyboard closes, otherwise you'll see flicker (while keyboard is closing, the part of the screen that it occupied will not be covered by backdrop).
I also tweaked the height of some existing modals to be relative to the visual viewport.
AFTER.MP4
Note
Would be nice to get someone to test this on Android, as I don't have an Android device to confirm the behaviour there. Shouldn't break anything though
Fixes #3939