feat: mobile ui and saved scroll position #1001
Conversation
Co-authored-by: Kevin Wu <[email protected]>
- remove styling from root div
There was a problem hiding this comment.
Broadly speaking, great PR on a much needed update. Separate even from moving tabs from top to bottom, really love that we're refactoring out our hardcoded styling values (which I'm definitely guilty of perpetuating).
Code-wise, I only have nits as to naming and comments. But since this is a "preview", it definitely has some UI problems (including, but not limited to):
- Calendar is broken -- tabs don't display right (height is probably not
100dvh) - Search Result is broken -- tabs don't display right (height is probably not
100dvh) - Padding is a little much (maybe
padding={0.5}would be better than1) - On iPhones, although
dvhis fantastic, we may want to add padding to the bottom as the iPhone "navigation bar" does still overlap on PWA
| import { Event, FormatListBulleted, MyLocation, Search } from '@mui/icons-material'; | ||
| import { Box, GlobalStyles, Paper, Stack, Tab, Tabs, Typography, useMediaQuery, useTheme } from '@mui/material'; |
There was a problem hiding this comment.
issue (style): tab text color is now blue when selected, rather the original white (contrast problem)
ap0nia
changed the title
ap0nia
changed the title
There was a problem hiding this comment.
Looks great, left two comments regarding styling:
- We're not accounting for header height, so 100dvh is too tall
- Our Mui v5 palette isn't correct, and it makes the tabs hard to read. Without expanding this PR to cover palette updates, I'd suggest using v4's tabs
There was a problem hiding this comment.
Beautiful, fabulous, tremendous!
I don't want to hold up this PR over a slight contrast problem (I'm happy to submit a PR following this one fixing it), so I'm going to approve
Feel free to merge or to rerequest review after looking at the tab contrast stuff, both are fine by me at this point since I think this PR is a big win for mobile users that I don't want held up
|
@KevinWu098, I'm going to take your word for it, but @ap0nia, do you know why the tests are failing? |
looks like this PR is the source: #1004 |
|
Oh, let me try tweaking the test to use real building numbers. |
|
| looks like this PR is the source: #1004 |
|
I handled possibly nullish values and updated the test snapshots. |
Summary
Save the scroll position of the previous tab when switching to a new tab, and restore the scroll position if the previous tab is revisited.
Move the tabs to the bottom and accommodate for mobile-specific factors like the bottom edge of the phone, rounded corners, etc.
Testing
Mobile Responsiveness
Ensure that the calendar and tabs are synchronized with their media queries.
Ensure that the elements are visually balanced at all points during the viewport shift.
Scroll Save
Project
Continuation of #999. I was originally trying to review it and figure out how I would actually implement it so i could provide feedback, but it ended up covering a larger scope.
Resolves #976