Fix: Add more space to bottom docked button by truph01 · Pull Request #44761 · Expensify/App

truph01 · 2024-07-03T05:48:19Z

Details

Fixed Issues

$ #44056
PROPOSAL: #44056 (comment)

Tests

Open app
Tap avatar
Tap display name
Observe the space at the bottom of docked button
There should be proper bottom SafeSpace below the button

Verify that no errors appear in the JS console

Offline tests

QA Steps

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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 verified the translation was requested/reviewed 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.js 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.
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

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2024-07-03T05:48:23Z

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

melvin-bot · 2024-07-03T07:23:47Z

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

shawnborton · 2024-07-03T08:16:27Z

Looking good, but here are spots where it's missing:

Wallet > Transfer Balance page
Workspace settings > Members > Invite new members

On the Edit expense details > Date screen, we have too much space below the Save button:

One interesting observation though... I feel like when the keyboard is up, we don't need the SafeSpace to show. Is there something we can do about that? Otherwise it feels like the gap below the Save button is way too large when the keyboard is visible.

truph01 · 2024-07-03T09:02:22Z

@shawnborton

Looking good, but here are spots where it's missing:
Wallet > Transfer Balance page
Workspace settings > Members > Invite new members

These screens do not use a form wrapper component, but the fix in this PR just applies to the bottom button in form wrapper component. If we need to fix these screen, we need to add the padding bottom to its bottom button manually. WDYT?

On the Edit expense details > Date screen, we have too much space below the Save button:

I fixed it

One interesting observation though... I feel like when the keyboard is up, we don't need the SafeSpace to show. Is there something we can do about that? Otherwise it feels like the gap below the Save button is way too large when the keyboard is visible.

I fixed it. Below is the result:

Screen.Recording.2024-07-03.at.15.59.10.mov

shawnborton · 2024-07-03T09:13:34Z

we need to add the padding bottom to its bottom button manually. WDYT?

I think we should add the padding there for consistency. And we should check as many other pages as we can too to make sure we aren't missing any.

I fixed it. Below is the result:

Much better!

allgandalf · 2024-07-03T18:11:47Z

src/components/Form/FormWrapper.tsx

+    useEffect(() => {
+        const keyboardWillShowListener = Keyboard.addListener('keyboardWillShow', () => {


What does this useEffect do exactly?

truph01 · 2024-07-04T11:12:12Z

@allgandalf I updated PR. Now, I use the additional padding bottom style in FormAlertWithSubmitButton directly instead of styling <FormAlertWithSubmitButton containerStyles={[..., additionalPaddingBottomStyle]} ... /> in FormWrapper as I did before because, there is a lot of screens we do not use FormWrapper but still have docked button like mentioned above:

Looking good, but here are spots where it's missing:
Wallet > Transfer Balance page
Workspace settings > Members > Invite new members

src/hooks/useExtraSafePaddingBottomStyle.ts

allgandalf · 2024-07-09T09:34:28Z

I will review this today

allgandalf

also are we covering all the cases with these changes?

src/hooks/useExtraSafePaddingBottomStyle.ts

truph01 · 2024-07-10T12:28:09Z

also are we covering all the cases with these changes?

Yes

allgandalf · 2024-07-10T12:32:37Z

src/hooks/useExtraSafePaddingBottomStyle.ts

+
+const useExtraSafePaddingBottomStyle = () => {
+    const styles = useThemeStyles();
+    const [willKeyboardShow, setWillKeyboardShow] = useState(false);


Can you typecast the boolean type of the variable and also rename the variable to something more readable like:

isKeyboardVisible?

I added the type for it. But I don't think we should rename the state, because as I mentioned above, willKeyboardShow is used to fix the issue didKeyboardShow encountered: There is a delay when the keyboard is visible by the user and the when didKeyboardShow is set to true. So it is not just isKeyboardVisible.

shawnborton · 2024-07-23T10:31:25Z

Yeah, that looks better. Can you help me understand why the two of them are different though?

allgandalf · 2024-07-23T10:32:45Z

The one i attached here are of the onboarding flow, and the one i attached here is of the display name (Both are from staging environment not related to our PR here but that really felt odd to me seeing so much bottom padding)

allgandalf · 2024-07-23T10:34:25Z

Yeah, that looks better. Can you help me understand why the two of them are different though?

We use KeyboardAvoidingView for the onboarding flow:

App/src/pages/OnboardingPersonalDetails/BaseOnboardingPersonalDetails.tsx

Lines 140 to 145 in 0d70442

    
           <KeyboardAvoidingView 
        
               style={[styles.flex1, styles.dFlex]} 
        
               behavior="padding" 
        
           > 
        
               <FormProvider 
        
                   style={[styles.flexGrow1, shouldUseNarrowLayout && styles.mt5, styles.mb5, shouldUseNarrowLayout ? styles.mh8 : styles.mh5]}

And screenwrapper for the display name page:

App/src/pages/settings/Profile/DisplayNamePage.tsx

Lines 72 to 77 in 0d70442

    
           <ScreenWrapper 
        
               includeSafeAreaPaddingBottom={false} 
        
               shouldEnableMaxHeight 
        
               testID={DisplayNamePage.displayName} 
        
           > 
        
               <HeaderWithBackButton

allgandalf

changes LGTM, looks good to be merged into staging

shawnborton · 2024-07-23T11:10:47Z

Got it. Yeah, ideally we use whatever is consistent with other places (like display name). How do you recommend we fix that?

allgandalf · 2024-07-23T11:13:57Z

Got it. Yeah, ideally we use whatever is consistent with other places (like display name). How do you recommend we fix that?

Use screenwrapper instead of KeyboardAvoidingView, but there must be a reason they added it there, I will open up a discussion on the PR which added that (SWM worked on the implementation so we would surely get a reply to that) will tag you there

shawnborton · 2024-07-23T11:58:40Z

Sounds good!

src/hooks/useExtraSafePaddingBottomStyle.ts

puneetlath · 2024-07-23T14:29:23Z

@Expensify/design someone mind reviewing as well?

puneetlath · 2024-07-23T14:29:49Z

Oh whoops, my bad. I see @shawnborton already reviewed above.

truph01 · 2024-07-24T15:08:07Z

@puneetlath I addressed your comment above

puneetlath

Sorry a couple more small requests.

puneetlath · 2024-07-24T15:16:34Z

src/hooks/useSafePaddingBottomStyle.ts

+import useStyledSafeAreaInsets from './useStyledSafeAreaInsets';
+import useThemeStyles from './useThemeStyles';
+
+const useSafePaddingBottomStyle = () => {


Can we add some general documentation above about what this hook is for.

Sorry for being a stickler, but I'm finding this comment a bit hard to follow. In the future, when should someone use this hook? Maybe if you can just explain that to me in words in a comment here, I can help you craft a code comment that is a bit more clear.

agree with you @puneetlath , @truph01 can you please follow up with this

@puneetlath

In the future, when should someone use this hook

This hook, useSafePaddingBottomStyle, is useful for adding extra bottom padding to a component based on the device's safe area. If the device's safe area padding bottom is 0, the hook returns 0. Otherwise, it provides a padding bottom of 20. Use this to ensure content visibility and layout consistency across different devices.

That's beautiful. Very clear! Let's make that the comment.

src/components/FormAlertWithSubmitButton.tsx

truph01 · 2024-07-24T15:49:11Z

All comment are resolved

puneetlath · 2024-07-24T20:03:35Z

src/hooks/useSafePaddingBottomStyle.ts

+import useStyledSafeAreaInsets from './useStyledSafeAreaInsets';
+import useThemeStyles from './useThemeStyles';
+
+const useSafePaddingBottomStyle = () => {


Sorry for being a stickler, but I'm finding this comment a bit hard to follow. In the future, when should someone use this hook? Maybe if you can just explain that to me in words in a comment here, I can help you craft a code comment that is a bit more clear.

src/hooks/useSafePaddingBottomStyle.ts

Co-authored-by: Puneet Lath <puneet@expensify.com>

truph01 · 2024-07-25T16:12:17Z

I updated PR @puneetlath

OSBotify · 2024-07-25T17:32:40Z

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

OSBotify · 2024-07-26T05:36:17Z

🚀 Deployed to staging by https://github.com/puneetlath in version: 9.0.13-0 🚀

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

OSBotify · 2024-07-29T18:39:29Z

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

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

Fix: Add more space to bottom docked button

45ce16e

truph01 marked this pull request as ready for review July 3, 2024 07:23

truph01 requested a review from a team as a code owner July 3, 2024 07:23

melvin-bot bot requested review from allgandalf and removed request for a team July 3, 2024 07:23

truph01 mentioned this pull request Jul 3, 2024

[HOLD for payment 2024-08-05] [$250] Bottom docked button views does not use correct SafeSpace in mobile #44056

Closed

6 tasks

shawnborton added the Ready To Build label Jul 3, 2024

This comment has been minimized.

Sign in to view

truph01 added 2 commits July 3, 2024 15:26

Fix: IOU request step screen

731fce3

Fix: Remove additional padding bottom if keyboard is shown

0385749

Fix: Lint

6f1fe83

allgandalf reviewed Jul 3, 2024

View reviewed changes

truph01 added 5 commits July 4, 2024 17:35

Fix: Create useExtraSafePaddingBottomStyle

b1d80d2

Fix: Merge main

01f07a2

Fix: extraPaddingBottomStyle logic

1a12356

Fix: Move extra padding bottom to FormAlertWithSubmitButton

abeea96

Fix: Remove unsed changes

1a62cc3

Fix: Lint

d38e7cb

allgandalf reviewed Jul 4, 2024

View reviewed changes

src/hooks/useExtraSafePaddingBottomStyle.ts Show resolved Hide resolved

allgandalf reviewed Jul 9, 2024

View reviewed changes

src/hooks/useExtraSafePaddingBottomStyle.ts Outdated Show resolved Hide resolved

allgandalf reviewed Jul 10, 2024

View reviewed changes

Feat: Merge maiin

7154df6

allgandalf approved these changes Jul 23, 2024

View reviewed changes

melvin-bot bot requested a review from puneetlath July 23, 2024 10:42

puneetlath requested changes Jul 23, 2024

View reviewed changes

src/hooks/useExtraSafePaddingBottomStyle.ts Outdated Show resolved Hide resolved

truph01 added 2 commits July 24, 2024 21:59

merge main

66d446f

Feat: Rename useExtraSafePaddingBottomStyle to useSafePaddingBottomStyle

bd01400

puneetlath requested changes Jul 24, 2024

View reviewed changes

allgandalf reviewed Jul 24, 2024

View reviewed changes

src/components/FormAlertWithSubmitButton.tsx Outdated Show resolved Hide resolved

Feat: Add comment to hooks and rename variable

0f06221

puneetlath requested changes Jul 24, 2024

View reviewed changes

puneetlath requested changes Jul 25, 2024

View reviewed changes

src/hooks/useSafePaddingBottomStyle.ts Outdated Show resolved Hide resolved

Update comment src/hooks/useSafePaddingBottomStyle.ts

b2844e0

Co-authored-by: Puneet Lath <puneet@expensify.com>

puneetlath approved these changes Jul 25, 2024

View reviewed changes

puneetlath merged commit aba404e into Expensify:main Jul 25, 2024

github-actions bot mentioned this pull request Jul 26, 2024

Deploy Checklist: New Expensify 2024-07-26 #46264

Closed

48 tasks

bernhardoj mentioned this pull request Aug 13, 2024

Android - Switch to Expesify classic - Next button has extra padding on 2nd screen of survey #47280

Closed

6 tasks

kirillzyusko mentioned this pull request Nov 26, 2024

fix: messages content overlap when bottom sheet is shown #42143

Merged

50 tasks

		useEffect(() => {
		const keyboardWillShowListener = Keyboard.addListener('keyboardWillShow', () => {

Conversation

truph01 commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Jul 3, 2024

Uh oh!

melvin-bot bot commented Jul 3, 2024

Uh oh!

This comment has been minimized.

shawnborton commented Jul 3, 2024

Uh oh!

truph01 commented Jul 3, 2024

Uh oh!

shawnborton commented Jul 3, 2024

Uh oh!

allgandalf Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

truph01 commented Jul 4, 2024

Uh oh!

Uh oh!

allgandalf commented Jul 9, 2024

Uh oh!

allgandalf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

truph01 commented Jul 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawnborton commented Jul 23, 2024

Uh oh!

allgandalf commented Jul 23, 2024

Uh oh!

allgandalf commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allgandalf left a comment

Choose a reason for hiding this comment

Uh oh!

shawnborton commented Jul 23, 2024

Uh oh!

allgandalf commented Jul 23, 2024

Uh oh!

shawnborton commented Jul 23, 2024

Uh oh!

Uh oh!

puneetlath commented Jul 23, 2024

Uh oh!

puneetlath commented Jul 23, 2024

Uh oh!

truph01 commented Jul 24, 2024

Uh oh!

puneetlath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

truph01 commented Jul 24, 2024

truph01 commented Jul 3, 2024 •

edited

Loading

allgandalf Jul 3, 2024 •

edited

Loading

allgandalf commented Jul 23, 2024 •

edited

Loading