Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.35k stars 2.78k forks source link

[$125] The continue button has some extra bottom padding on the page we enter name after selecting the onboarding option. #47087

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.18-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @allgandalf Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1723118907902239

Action Performed:

  1. login with new account
  2. On onboarding modal, select something else
  3. Observe the extra bottom padding

Expected Result:

No extra bottom padding for Continue button

Actual Result:

Extra bottom padding observed for Continue button

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0112ea720a987767e3
  • Upwork Job ID: 1821573061746870352
  • Last Price Increase: 2024-08-15
  • Automatic offers:
    • allgandalf | Reviewer | 103622446
    • BhuvaneshPatil | Contributor | 103622448
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @BhuvaneshPatil
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

allgandalf commented 1 month ago

[!NOTE] I can take over as C+ here as i reported this issue and have context @joekaufmanexpensify

dominictb commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-08 15:36:41 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

The continue button has some extra bottom padding on the page we enter name after selecting the onboarding option.

What is the root cause of that problem?

We are using the redundant component: https://github.com/Expensify/App/blob/cc4626f393daf4171a0bccc190be39132ac021ea/src/pages/OnboardingPersonalDetails/BaseOnboardingPersonalDetails.tsx#L149

When footerContent is used, additional padding is added: https://github.com/Expensify/App/blob/cc4626f393daf4171a0bccc190be39132ac021ea/src/components/FormAlertWithSubmitButton.tsx#L82 that leads to the current bug "Extra bottom padding observed for Continue button".

Also, I said that it is redundant component because the OfflineIndicator is already handled in ScreenWrapper.

What changes do you think we should make in order to solve the problem?

We need to remove that line. Also we should add additional prop to ScreenWrapper: offlineIndicatorStyle={styles.mtAuto}. We should check and fix other similar screen in onboarding flow.

github-actions[bot] commented 1 month ago

aasingh-at-ox Your proposal will be dismissed because you did not follow the proposal template.

dominictb commented 1 month ago

Proposal updated

dominictb commented 1 month ago

Proposal updated

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0112ea720a987767e3

melvin-bot[bot] commented 1 month ago

Current assignee @allgandalf is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 month ago

Upwork job price has been updated to $125

joekaufmanexpensify commented 1 month ago

Price updating as this is a pretty minor issue

allgandalf commented 1 month ago

@dominictb , can you please state RCA, the current one:

We are using the redundant component:

Doesn't really explain RCA, also an advice, do not copy paste the issue title for the first Section, that is meant for the contributor to express what they understand from current issue

dominictb commented 1 month ago

@allgandalf Thanks for your notes. I updated my RCA.

BhuvaneshPatil commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-08 17:01:11 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Continue button in on-boarding forms has extra space at bottom when viewed on devices with small screen width

What is the root cause of that problem?

The extra margin bottom is being added when we have footer content passed in FormProvider component https://github.com/Expensify/App/blob/cc4626f393daf4171a0bccc190be39132ac021ea/src/components/FormAlertWithSubmitButton.tsx#L82

Although OfflineIndicator renders when we are online null but margins is still applied because it's truthy value.

const PersonalDetailsFooterInstance = <OfflineIndicator />;

What changes do you think we should make in order to solve the problem?

There are two approaches for solving this -

  1. Remove the line where we pass FooterContent to FormProvider and add that component outside form provider -
    <FormProvider
                    style={[styles.flexGrow1, isMediumOrLargerScreenWidth && styles.mt5, isMediumOrLargerScreenWidth ? styles.mh8 : styles.mh5]}
                    formID={ONYXKEYS.FORMS.ONBOARDING_PERSONAL_DETAILS_FORM}
                    validate={validate}
                    onSubmit={completeEngagement}
                    submitButtonText={translate('common.continue')}
                    enabledWhenOffline
                    submitFlexEnabled
                    shouldValidateOnBlur={false}
                    shouldValidateOnChange={shouldValidateOnChange}
                    shouldTrimValues={false}
                >
                 {....}
                </FormProvider>
                {isSmallScreenWidth && PersonalDetailsFooterInstance}

    This approach is being followed at other places in application.

    Create room page
Screenshot 2024-08-08 at 10 17 44β€―PM

  1. This approach is simpler to follow and with less changes. If we change PersonalDetailsFooterInstance to
    const PersonalDetailsFooterInstance = OfflineIndicator({});

    It will work as expected. Reason being function will return null in case we are online, so footerContent will be falsy value and extra margin not added. If we are offline, it will return the content of component.

Demo https://github.com/user-attachments/assets/4893113c-bed6-4bf3-955e-4545f1ca7656

Also we need to verify if removing footer content and then changing to offline throttling shows indicator as mentioned in other proposal because I didn't get the expected result.

What alternative solutions did you explore? (Optional)

BhuvaneshPatil commented 1 month ago

Proposal updated,

change online to offline at one place

allgandalf commented 1 month ago

Will review the proposal today

joekaufmanexpensify commented 1 month ago

Proposals pending review

joekaufmanexpensify commented 1 month ago

@allgandalf think there will be an update today?

allgandalf commented 1 month ago

Yeah reviewing now, sorry for the delay, this is not showing in my K2 for some reason and hence the delay :))

allgandalf commented 1 month ago

Let's go with @dominictb 's proposal their solution is correct, we already have the offline indicator in screenwrapper:

https://github.com/Expensify/App/blob/2d1647af7c74c121bbb47525026df7af65503a35/src/components/ScreenWrapper.tsx#L287

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @aldo-expensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

allgandalf commented 1 month ago

@BhuvaneshPatil @dominictb one note for both of you guys:

Please re-state the problem that we are trying to solve in this issue.

The above section is for you guys to explain what you understand about the problem and not copy paste the issue title, moving forward please take care to explain what you have understood from the issue:

BhuvaneshPatil commented 1 month ago

@allgandalf Can you please verify if removing https://github.com/Expensify/App/blob/cc4626f393daf4171a0bccc190be39132ac021ea/src/pages/OnboardingPersonalDetails/BaseOnboardingPersonalDetails.tsx#L149 Line is working. Because I tried and it didn't show the indicator

https://github.com/user-attachments/assets/17fdc8d3-145e-4e4d-a961-8a423302228c

allgandalf commented 1 month ago

@BhuvaneshPatil , can you also pass the style offlineIndicatorStyle={styles.mtAuto} as mentioned in @dominictb 's proposal? Do let me know if you still do not see the indicator, IMO it should work:

Screenshot 2024-08-14 at 8 26 32β€―PM
BhuvaneshPatil commented 1 month ago

https://github.com/user-attachments/assets/e4508cba-35cb-44ef-a6e4-58b584b8ad99

Please verify changes I have shown are correct. It didn't show for me

allgandalf commented 1 month ago

from your code, I can see that you are passing the style as an array object:

Screenshot 2024-08-14 at 8 41 23β€―PM

That is the reason why you are not able see the indicator.

BhuvaneshPatil commented 1 month ago

Tried directly passing the style -

https://github.com/user-attachments/assets/a6d973d0-129f-42ab-8d5e-187beac2c094

That didn't work either

allgandalf commented 1 month ago

@BhuvaneshPatil can you please check on a simulator / emulator? I checked the solution again and it works well, here is the screenshot with the updated code:

Screenshot 2024-08-14 at 9 41 28β€―PM

I hope i cleared your doubt

BhuvaneshPatil commented 1 month ago

@allgandalf Checked on simulator, it is working. Interesting though why it's not working on mweb.

I think we should look for a solution that works for all small screens meaning native and mweb

joekaufmanexpensify commented 1 month ago

Waiting for @aldo-expensify sign off on proposal

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

aldo-expensify commented 1 month ago

It would have been nice if the screenshots in the issue showed what padding this is talking about. The pictures are the same. Is this something that happens only in IOS native? the issues says that, but then @BhuvaneshPatil says the proposal doesn't work in mWeb.... so is it only happening in IOS native or is it happening in all mobile screens? Can we get a more accurate description of the issue?

joekaufmanexpensify commented 1 month ago

@aldo-expensify I updated the screenshots in OP of the issue, I think applause incorrectly added the same one twice from the slack thread.

@allgandalf do you agree that the selected proposal is not working on all platforms?

aldo-expensify commented 1 month ago

@aldo-expensify I updated the screenshots in OP of the issue, I think applause incorrectly added the same one twice from the slack thread.

Thanks!

allgandalf commented 1 month ago

@allgandalf do you agree that the selected proposal is not working on all platforms?

I will test both the proposals on all platforms and revert back

joekaufmanexpensify commented 1 month ago

Great. TY!

allgandalf commented 1 month ago

Umm, i too checked and found that @dominictb 's proposal doesn't work on small screen web, the thing here is that on mWeb the indicator is indeed present it's just that we do not have enough padding to make it visible (On native, we get padding because of the notch present).

I checked at other places of the code and found that what @BhuvaneshPatil 's first solution suggested is the way we consistently follow in our app:

https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/workspace/WorkspaceNewRoomPage.tsx#L337-L338

https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/OnboardingPurpose/BaseOnboardingPurpose.tsx#L113-L114

So i am inclined to go with @BhuvaneshPatil 's 1st solution.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @aldo-expensify is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

joekaufmanexpensify commented 1 month ago

As an fyi, going to be OOO from tomorrow - Monday. Not adding another BZ since nothing should be needed during that time. Please ask in slack if something comes up.

allgandalf commented 1 month ago

Enjoy your time off πŸ–οΈ

joekaufmanexpensify commented 1 month ago

TY!

aldo-expensify commented 1 month ago

Thanks for rechecking everything @allgandalf

melvin-bot[bot] commented 1 month ago

πŸ“£ @allgandalf πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

πŸ“£ @BhuvaneshPatil πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 month ago

@BhuvaneshPatil @joekaufmanexpensify @aldo-expensify @allgandalf this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

aldo-expensify commented 1 month ago

@BhuvaneshPatil when do you think you can have a PR up?

BhuvaneshPatil commented 1 month ago

Feeling under the weather for 2-3 days. Please give me sometime. I will raise the PR.

aldo-expensify commented 1 month ago

No worries, just asking, this is a low priority issue, take your time until you feel better :)

joekaufmanexpensify commented 4 weeks ago

PR in progress

allgandalf commented 4 weeks ago

@BhuvaneshPatil hope you're doing well

BhuvaneshPatil commented 3 weeks ago

@allgandalf I am good now. Thank you for asking. I will be raising the PR by tonight