Fix Certinia prerequisites CTA being covered by offline indicator - #94085
Conversation
…offline indicator does not cover the CTA Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot In the OP, we can remove the comments within |
|
@MelvinBot PR Author checklist is still failing. Please add and tick all the checkboxes as per the template https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md |
|
@MelvinBot Please mark PR as "Ready for review" |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp94085-android-hybrid-001.mp4Android: mWeb Chrome94085-mweb-chrome-001.mp4iOS: HybridApp94085-ios-hybrid-001.mp4iOS: mWeb Safari94085-mweb-safari-001.mp4MacOS: Chrome / Safari94085-web-chrome-001.mp4 |
rojiphil
left a comment
There was a problem hiding this comment.
@arosiclair Code changes LGTM.
Approving for your review. Thanks.
rojiphil
left a comment
There was a problem hiding this comment.
approving again since pr reviewer checklist failed.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @arosiclair has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.4.17-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed this PR against the help site files under This change is a purely visual/layout bug fix — it adds
The existing Certinia connection docs already describe how to connect and configure the integration, and none of them reference the offline-indicator layout behavior this PR touches. So there's nothing to update on the help site, and no draft docs PR was created. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.17-3 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.17-3 🚀
|
Explanation of Change
On Step 3 (OAuth) of the Certinia prerequisites flow, going offline made the Connect to Certinia CTA appear to vanish instead of staying visible but greyed‑out/disabled.
The button was not being unmounted — it was being covered by the bottom‑docked offline indicator. When the device goes offline,
ScreenWrapperrenders the small‑screen offline indicator withstickToBottom(position: absolute; bottom: 0), and on devices with soft‑key/gesture nav it gets an opaque background. The Certinia CTA lives in aFixedFooterpushed flush to the bottom withmtAuto, and that footer did not opt into bottom safe‑area padding, so it reserved no room for the indicator — the indicator painted directly over the disabled button.This change adds
addBottomSafeAreaPaddingto theFixedFooter. BecauseuseBottomSafeSafeAreaPaddingStyledefaultsaddOfflineIndicatorBottomSafeAreaPaddingto the value ofaddBottomSafeAreaPadding, the single flag both adds safe‑area padding and reservesCONST.OFFLINE_INDICATOR_HEIGHTof space when offline — lifting the footer above the indicator so the CTA stays visible and greyed‑out. The offline‑indicator padding term is gated onisOffline, so online layout is unchanged. This matches the existing pattern used by the sibling Sage Intacct and QBD setup pages.The
isDisabled={isLastStep && isOffline}from PR #92826 is kept — both pieces are needed (one disables the button, this one keeps it visible).Fixed Issues
$ #93285
PROPOSAL: #93285 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari