Skip to content

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

Conversation

chrispader
Copy link
Contributor

@chrispader chrispader commented Mar 14, 2025

@mountiny @lakchote

Explanation of Change

This PR improves how we display the OfflineIndicator in the ScreenWrapper 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

  • Verify that no errors appear in the JS console

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:

  1. Go to a screen content where content flows all the way down to the bottom edge of the screen (e.g. Workspace -> Categories)
  2. Enable edgeToEdgeBottomSafeAreaPadding on the ScreenWrapper component in code
  3. Add addBottomSafeAreaPadding to one of either ScrollView/SelectionList/FormProvider or whatever component contains the scrollable content
  4. The new offline indicator behaviour should be enabled by default if edgeToEdgeBottomSafeAreaPadding is set to true.
  5. Check that it looks the same as in the screen recordings

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

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Soft keys navigation Gesture bar
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
Before After
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 Screenshot 2025-03-14 at 17 36 06 Screenshot 2025-03-14 at 17 36 10

@chrispader chrispader requested review from a team as code owners March 14, 2025 16:35
Copy link

melvin-bot bot commented Mar 14, 2025

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

@melvin-bot melvin-bot bot requested review from youssef-lr and removed request for a team March 14, 2025 16:35
Copy link

melvin-bot bot commented Mar 14, 2025

@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]

@chrispader
Copy link
Contributor Author

chrispader commented Mar 14, 2025

@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)

@mountiny
Copy link
Contributor

Waiting for the design team but @hungvu193 feel free to proceed with testing and a review

Copy link
Contributor

🚧 @mountiny has triggered a test app build. You can view the workflow run here.

Copy link
Contributor

🚧 @mountiny has triggered a test app build. You can view the workflow run here.

This comment has been minimized.

@hungvu193
Copy link
Contributor

@chrispader In order to enable enableEdgeToEdgeBottomSafeAreaPadding, I need to replace enableEdgeToEdgeBottomSafeAreaPadding = false default value to true right? Is there any place that I need to update? I'm asking because when I changed the enableEdgeToEdgeBottomSafeAreaPadding = true, the OfflineIndicator for screens inside BottomNavigationStack looks off

Screen.Recording.2025-03-18.at.22.37.56.mov

@chrispader
Copy link
Contributor Author

chrispader commented Mar 18, 2025

In order to enable enableEdgeToEdgeBottomSafeAreaPadding, I need to replace enableEdgeToEdgeBottomSafeAreaPadding = false default value to true right?

@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 ScreenWrappers.

On whatever screen you want to test it, just add the enableEdgeToEdgeBottomSafeAreaPadding prop

Lmk if this is still a problem when you enable this selectively

@hungvu193
Copy link
Contributor

@chrispader Ah Ok, got it. For example with screen inside BottomNavigation like BaseSidebarScreen, which props should I enable beside enableEdgeToEdgeBottomSafeAreaPadding?

@chrispader
Copy link
Contributor Author

chrispader commented Mar 18, 2025

@chrispader Ah Ok, got it. For example with screen inside BottomNavigation like BaseSidebarScreen, which props should I enable beside enableEdgeToEdgeBottomSafeAreaPadding?

additionally you'll need to add addBottomSafeAreaPadding to whatever component is used for the scrollable content.

E.g. for BaseSidebarScreen, it's nested in SidebarLinksData -> Sidebar -> LHNOptionsList -> FlashList... But now that i see it, i haven't added this prop to FlashList yet, so you cannot really test it there yet

@chrispader chrispader closed this Mar 18, 2025
@chrispader chrispader reopened this Mar 18, 2025
@chrispader
Copy link
Contributor Author

Sorry closed this PR accidentally 😵‍💫

@chrispader
Copy link
Contributor Author

chrispader commented Mar 18, 2025

We're only using FlashList in two locations though, so i guess i won't create a wrapper component for this case.

E.g. for BaseSidebarScreen, it's nested in SidebarLinksData -> Sidebar -> LHNOptionsList -> FlashList... But now that i see it, i haven't added this prop to FlashList yet, so you cannot really test it there yet

@hungvu193 if you really want to test the BaseSidebarScreen, you can replace the contentContainerStyles with the following in FlashList

const contentContainerStyle = useBottomSafeSafeAreaPaddingStyle({addBottomSafeAreaPadding: true, style: contentContainerStyles});

@hungvu193
Copy link
Contributor

We're only using FlashList in two locations though, so i guess i won't create a wrapper component for this case.

E.g. for BaseSidebarScreen, it's nested in SidebarLinksData -> Sidebar -> LHNOptionsList -> FlashList... But now that i see it, i haven't added this prop to FlashList yet, so you cannot really test it there yet

@hungvu193 if you really want to test the BaseSidebarScreen, you can replace the contentContainerStyles with the following in FlashList

const contentContainerStyle = useBottomSafeSafeAreaPaddingStyle({addBottomSafeAreaPadding: true, style: contentContainerStyles});

Thanks. Got it 👍

Copy link
Contributor

@hungvu193 hungvu193 left a 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 👍

mountiny
mountiny previously approved these changes Mar 18, 2025
Copy link
Contributor

@mountiny mountiny left a 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

@hungvu193
Copy link
Contributor

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

@chrispader chrispader changed the title fix: Better OfflineIndicator bottom safe area handling [NO QA] fix: Better OfflineIndicator bottom safe area handling Mar 18, 2025
@chrispader
Copy link
Contributor Author

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

@hungvu193 is correct, i guess QA still needs to test that nothing changed? cc @mountiny

I updated the QA steps, should we remove [NO QA] in this case?

@shawnborton
Copy link
Contributor

Let us know if you need help from the design team for testing.

@mountiny mountiny changed the title [NO QA] fix: Better OfflineIndicator bottom safe area handling fix: Better OfflineIndicator bottom safe area handling Mar 18, 2025
Copy link
Contributor

@mountiny mountiny left a 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

@mountiny
Copy link
Contributor

@shawnborton I believe no need for now

Copy link
Contributor

@lakchote lakchote left a comment

Choose a reason for hiding this comment

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

LGTM

@mountiny
Copy link
Contributor

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

@mountiny mountiny merged commit 9d2f0fe into Expensify:main Mar 19, 2025
20 checks passed
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Copy link
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.16-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅
🤖🔄 android HybridApp 🤖🔄 success ✅
🍎🔄 iOS HybridApp 🍎🔄 success ✅

Copy link
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 9.1.16-4 🚀

platform result
🤖 android 🤖 true ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅
🤖🔄 android HybridApp 🤖🔄 failure ❌
🍎🔄 iOS HybridApp 🍎🔄 failure ❌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants