-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: Better OfflineIndicator
bottom safe area handling
#58507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Better OfflineIndicator
bottom safe area handling
#58507
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@youssef-lr Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@Expensify/design could you please confirm that the screen recordings for Android, iOS and mWeb look good to you. (attached in the PR description) As described in the PR description, i made the offline indicator stick to to the navigation bar and added translucent background. On Android with soft keys, this will basically "extend" the navigation bar, whereas with gesture bar it will add translucent background behind the navigation bar too. Imo, this is the only way to properly have the offline indicator sticking to the bottom, without adding back background color to bottom edge. Without this approach, once the user goes offline the bottom area of the screen will (again) be filled with background, so that effectively edge-to-edge scrolling is not possible... On web/mWeb, do you think the offline indicator should also be translucent for consistency or should it just have full opaque white background like before? (I attached mWeb Safari screen recordings with a before vs. after) |
Waiting for the design team but @hungvu193 feel free to proceed with testing and a review |
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
@chrispader In order to enable Screen.Recording.2025-03-18.at.22.37.56.mov |
@hungvu193 no you need to enable it on a per-screen basis. Changing the default value will mess up with the bottom safe area handling on other screens or in nested On whatever screen you want to test it, just add the Lmk if this is still a problem when you enable this selectively |
@chrispader Ah Ok, got it. For example with screen inside BottomNavigation like |
additionally you'll need to add E.g. for |
Sorry closed this PR accidentally 😵💫 |
We're only using
@hungvu193 if you really want to test the const contentContainerStyle = useBottomSafeSafeAreaPaddingStyle({addBottomSafeAreaPadding: true, style: contentContainerStyles}); |
Thanks. Got it 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrispader can you update the qa as noqa as it seems like changes in code are required to test this
Same with title
I think QA still need to verify the current offline indicator works as expected. We only need to change code to test the new offline indicator |
OfflineIndicator
bottom safe area handlingOfflineIndicator
bottom safe area handling
@hungvu193 is correct, i guess QA still needs to test that nothing changed? cc @mountiny I updated the QA steps, should we remove |
Let us know if you need help from the design team for testing. |
OfflineIndicator
bottom safe area handlingOfflineIndicator
bottom safe area handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok great thanks, I have updated the text to just make sure to be careful with those, but also I think they will just raise any padding issues in general
@lakchote @youssef-lr all yours
@shawnborton I believe no need for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Not going to hold this one for much longer just to make sure there are no conflicts. thanks for the work on this @chrispader Lets continue to wrap this up and possibly few lines into the Forms doc and some other markdown files might be helpful |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.16-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.1.16-4 🚀
|
@mountiny @lakchote
Explanation of Change
This PR improves how we display the
OfflineIndicator
in theScreenWrapper
and adds logic to adapt the bottom safe area padding based on whether the offline indicator is visible. This PR also cleans up code around styling, etc.With this change, the offline indicator will stick to the bottom and when active, it will have a translucent background, that goes all the way to the bottom edge.
With Android soft keys navigation, this basically extends the already translucent navigation bar. When we have a gesture bar, this will also add the translucent background behind the gesture bar.
(Screenshots attached)
Fixed Issues
$ #53186
PROPOSAL:
Tests
This PR introduces new logic that is not enabled by default. Also, the changes to the
OfflineIndicator
+ScreenWrapper
component should not cause any changes in UI. Therefore, there should be no changes in app behaviour.Therefore we need to test that the UI still behaves as usual.
To test the new offline indicator bottom safe area handling:
edgeToEdgeBottomSafeAreaPadding
on theScreenWrapper
component in codeaddBottomSafeAreaPadding
to one of eitherScrollView
/SelectionList
/FormProvider
or whatever component contains the scrollable contentedgeToEdgeBottomSafeAreaPadding
is set to true.Offline tests
QA Steps
This PR introduces new logic that is not enabled by default. Also, the changes to the
OfflineIndicator
+ScreenWrapper
component should not cause any changes in UI. Therefore, there should be no changes in app behaviour.QA only needs to make sure that the UI still behaves as usual and that the offline indicator positioning and styling did not change.
So there is nothing specific to QA here, but just keep an eye out on the bottom padding throughout the regression testing
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen_Recording_20250314_172752_New.Expensify.Dev.mp4
Screen_Recording_20250314_172935_New.Expensify.Dev.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-03-14.at.19.01.39.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-03-15.at.11.51.03.mp4
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-03-15.at.11.50.20.mp4
MacOS: Chrome / Safari