feat: mobile ui and saved scroll position #1001
Conversation
Co-authored-by: Kevin Wu <kevinwu098@gmail.com>
- 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
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. |
There was a problem hiding this comment.
Oops I didn't see that @KevinWu098 had a comment that hasn't been resolved, and it does actually matter.
|
Which one? The white color on the tabs? I already fixed that, forgot to resolve it. Any other theming issues are due to the differences in MUI version, which should be resolved separately. Oh wait, need to fix map issue. |
|
Wait, is the map broken on live? It looks weird on my side ... |
|
Ok, I think it was because the CSS from leaflet wasn't imported. Should be sorted out now. For reference, this is what I see on my screen for the live site:
|
If you updated the MUI version, and it changed the color undesirably, than the PR would be atomic if you changed the code such that that does not occur. |
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
Resolves #1016 by optional chaining the custom event location's name.