-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: new way to handle insets in the app #53250
feat: new way to handle insets in the app #53250
Conversation
Co-authored-by: edug <gedu@users.noreply.github.com>
stepping out quickly, will wrap that PR up in a few hours! |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
BUG Save button not consistent across screens Screen.Recording.2024-11-29.at.4.53.45.AM.mov |
Fixed in 9901700 |
Alright, I believe this PR is ready for another round of review 😊 |
Fixed @ZhenjaHorbach ❤️ |
One more 2024-11-29.20.26.56.mov2024-11-29.20.28.11.mov |
@mountiny |
@ZhenjaHorbach @allroundexperts please add the checklist whoever is ready and we can merge this one |
Reviewer Checklist
Screenshots/VideosAndroid: Nativehttps://drive.google.com/drive/folders/1wfbYDmllorbgSoFgr3_MF3vpWcMrh4r2?usp=sharing Android: mWeb Chromehttps://drive.google.com/drive/folders/1iBHleN7bEogGbCSwsaD_6JJgRATYsBqS?usp=sharing iOS: mWeb Safarihttps://drive.google.com/drive/folders/1iBHleN7bEogGbCSwsaD_6JJgRATYsBqS?usp=sharing MacOS: Chrome / Safarihttps://drive.google.com/drive/folders/1rXHvup-hIdAYDQU8mtL8dUaqFkO_BrOK?usp=sharing |
We have some eslint issues so LGTM ! |
Merge conflicts |
ESLint failing because of the useOnyx so that is fine on such PR
|
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.
One NAB comment, thanks for clearing this up and thanks @allroundexperts and @ZhenjaHorbach for review and testing
<FormElement | ||
key={formID} | ||
ref={formContentRef} | ||
style={[style, safeAreaPaddingBottomStyle.paddingBottom ? safeAreaPaddingBottomStyle : styles.pb5]} | ||
// Note: the paddingBottom is only grater 0 if no parent has applied the inset yet: |
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.
// Note: the paddingBottom is only grater 0 if no parent has applied the inset yet: | |
// Note: the paddingBottom is only greater than 0 if no parent has applied the inset yet: |
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Not emergency, I have explained above why the test has been failing |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Explanation of Change
Motivation for changes
We recently enabled edge-to-edge mode on android:
which caused some regressions, mostly related to the bottom navigation bar and the content either having too much or to less spacing. As well as problems where when opening the keyboard the content would be either hidden behind the keyboard or would have too much spacing between the keyboard's edge.
This PR aims to fix all of those UI regressions
Problems with current implementation
The main problem for all of this is that there are different paths to handle safe area insets. As of now there are:
useSafeAreaBottomInset()
SafeAreaConsumer
useSafeAreaInsets()
Every screen and their child components was making their own guessing about how the bottom safe area should be handled, leading to inconsistencies. They have special conditions for different platforms, and combining them creates even more inconsistent results.
The strategy of this PR
Introduce an API that unifies the safe area handling, provides an equal interface for any platform and creates the same visual output across platforms.
Postulates:
Changes:
<ScreenWrapper>
component should handle the bottom safe area inset in the most places.ScreenWrapperStatusContext
we added whether a top or bottom padding has been applieduseStyledSafeAreaInsets
.0
values, ensuring child components don't risk adding safe area padding multiple timesEarlier the bottom safe area inset was handled by the
FormProvider
. This has shifted now to<ScreenWrapper>
, so we changed a lot of to enable the inset:<ScreenWrapper includeSafeAreaPaddingBottom />
(where before it was false)FormProvider
shows a button at the bottom. If the button adds thepaddingBottom
space for the safe area, and you open the keyboard, the safe area spacing will be visible between the keyboard and the button, creating too much space. Thus the parent screen should add thepaddingBottom
Future
useSafeAreaBottomInset
andSafeAreaConsumer
in this PR. We want to look into all of their usages carefully and eventually remove them entirelyScreenWrapper
'sincludeSafeAreaPaddingBottom
prop. If they need to customize the behaviour they should useuseSafeAreaBottomInset
Fixed Issues
$ #53183
$ #53207
$ #53217
$ #53221
$ #53192
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
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
telegram-cloud-document-2-5354803205601516843.mp4
Android: mWeb Chrome
telegram-cloud-document-2-5354803205601516836.mp4
iOS: Native
new-insets-trimmed.mp4
iOS: mWeb Safari
ScreenRecording_11-29-2024.6-27-34.PM_1.MP4
MacOS: Chrome / Safari
MacOS: Desktop