Skip to content
This repository was archived by the owner on Jul 27, 2025. It is now read-only.

Conversation

@KenTandrian
Copy link
Contributor

@KenTandrian KenTandrian commented Jun 18, 2025

This PR fixes #2342, which comprises these issues:

  • The page content was not taking full height of the iOS screen in PWA, resulting in dead space at the bottom.
  • The top status bar and the bottom home indicator were not theme-aware, breaking the immersive dark mode experience.

Changes

  • Set env(safe-area-inset-top) as top margin instead of top padding for html.
  • Remove safe-area-inset-bottom padding from the bottom navigation component since html has it.
  • Ensured body adapts background color by dynamically setting it to bg-surface.
  • Add top and bottom padding based on safe area height to dialog component.

After

Dark mode Light mode

@KenTandrian KenTandrian marked this pull request as draft June 18, 2025 16:00
@KenTandrian KenTandrian marked this pull request as ready for review June 18, 2025 17:31
Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix here, but this seems to cause several visual regressions to the desktop version of the app. Below are just a few examples, but I'm sure there are several more:

CleanShot 2025-06-23 at 10 27 27

CleanShot 2025-06-23 at 10 28 08

We need to be super careful editing these files as they apply to many views across the app.

@KenTandrian
Copy link
Contributor Author

KenTandrian commented Jun 23, 2025

@zachgoll Thanks for the feedback. My bad for removing h-full from body, resulting in the page not taking full height.

I have reverted some of the changes and only focus on 4 things:

  1. Set env(safe-area-inset-top) as top margin instead of top padding for html.
  2. Remove safe-area-inset-bottom padding from the bottom navigation component since html has it.
  3. Ensured body adapts background color by dynamically setting it to bg-surface.
  4. Add top and bottom padding based on safe area height to dialog component.

Edit: added dialog padding.

@KenTandrian KenTandrian requested a review from zachgoll June 23, 2025 17:22
@albertorizzi
Copy link

@KenTandrian can you fix also modals? I can't close.

image

@KenTandrian
Copy link
Contributor Author

@albertorizzi done!

Dark mode Light mode

@albertorizzi
Copy link

Wow! Thanks a lot!😍

@albertorizzi
Copy link

Your PR #2410 fix also the Current Market Price "Unknown" in these screens?

@KenTandrian
Copy link
Contributor Author

Your PR #2410 fix also the Current Market Price "Unknown" in these screens?

Yes, I think that should be covered since @holding.security.current_price is retrieved from the same find_or_fetch_price method.

@albertorizzi
Copy link

Is it possible to improve the mobile view of the Holdings tab?

image

@KenTandrian
Copy link
Contributor Author

Is it possible to improve the mobile view of the Holdings tab?

I don't think it's related to PWA screen height? It should be a separate issue.

@KenTandrian
Copy link
Contributor Author

Hi @zachgoll, a gentle follow up on this PR.

I've pushed fixes to address the desktop regressions you pointed out and have also resolved the modal issue reported by @albertorizzi. I believe all requested changes are complete.

Please let me know if there's anything else needed when you have a moment. Thanks!

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @KenTandrian, sorry this is a bit of a tough one to review just given this will affect every page in our app.

I haven't had a chance to dig deeply into this yet, but my gut tells me we really need to keep that existing "global padding" intact on our html element, then apply specific padding/margin adjustments to things like our Dialog to make them look good on mobile. Changing the global property to margin introduces a lot of weird UI states on "full-bleed" sections as I had screenshotted earlier, so I don't think we should be using that here.

So I think in summary what we should probably do is:

  • Revert global CSS changes (keep existing padding safe areas)
  • Apply specific safe area padding to fixed elements like dialogs, mobile navs, etc.

@KenTandrian
Copy link
Contributor Author

Hey @zachgoll, it's tricky to implement this without touching the html element at all.

To illustrate why, I have reverted all my changes and added borders to highlight the default sizing:

  1. Green border: html
  2. Blue border: body
  3. Purple border: nav (mobile bottom nav)
Dark mode

 
As you can see from the screenshot, the html element isn't taking up the full height of the screen by default. This then prevents the body and nav elements from reaching the bottom as well. Because of this, I don't think changes isolated to just the dialog or mobile nav would be enough to resolve the layout issue.

Do you have any ideas on how we could address the html element's height?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: PWA iOS Not Fullscreen / Not Theme Aware

4 participants