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
2.99k stars 2.5k forks source link

[$250] "Next" and "split" button moves a bit when navigate back #41855

Open m-natarajan opened 1 week ago

m-natarajan commented 1 week 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: 1.4.71-4 Reproducible in staging?: Yes Reproducible in production?: Yes 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: @youssef-lr Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1715122550547929

Action Performed:

  1. Launch the app
  2. Tap the fab > Split expense
  3. Enter amount and Choose from contacts
  4. Tap Next > Tap the back arrow
  5. Tap Next again

    Expected Result:

    No visual issues when navigating back and forth

    Actual Result:

    After step 4 and 5 "Next" and "Split" button moves

    Workaround:

    Unknown

    Platforms:

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

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [X] iOS: Native
    • [ ] iOS: mWeb Safari
    • [ ] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/38435837/29d10bfc-a168-4e55-85e2-75ae331a6ffd

https://github.com/Expensify/App/assets/38435837/2b45f179-abe9-48de-a10d-6c83254ca83e

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01419f469e7473b289
  • Upwork Job ID: 1789073733749583872
  • Last Price Increase: 2024-05-10
  • Automatic offers:
    • Ollyws | Reviewer | 0
    • bernhardoj | Contributor | 0
Issue OwnerCurrent Issue Owner: @Ollyws
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @alexpensify (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.

bernhardoj commented 1 week ago

Proposal

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

The next and split button moves down when navigating between pages while a keyboard is open.

What is the root cause of that problem?

The button is wrapped with FixedFooter. https://github.com/Expensify/App/blob/807c945ef8edabe86cc62f16c866aa754db8750b/src/components/FixedFooter.tsx#L17-L29

The confirmation page uses BaseOptionsSelector which uses FixedFooter.

In FixedFooter, we add a bottom padding if the keyboard shown state is true. When we move to the confirmation page while the keyboard is shown, both the padding-bottom from FixedFooter and the confirmation page itself are added. After the keyboard state is fully hidden, the padding-bottom from FixedFooter is gone, so the button moves down a bit.

In the money request participants page, we are using BaseSelectionList and only add the padding-bottom if the keyboard is hidden. The list footer also uses FixedFooter. https://github.com/Expensify/App/blob/807c945ef8edabe86cc62f16c866aa754db8750b/src/components/SelectionList/BaseSelectionList.tsx#L517-L518

So, when we open/close the keyboard, you will see the button move up/down based on the keyboard state.

I believe the reason we add the padding-bottom in FixedFooter only when the keyboard is shown is because the padding-bottom in BaseSelectionList is gone when the keyboard is shown. It's to replace the removed padding-bottom so the list footer content has a padding-bottom.

Without padding-bottom (if we remove the padding-bottom logic from FixedFooter):

image

The reason we don't apply padding-bottom in BaseSelectionList when the keyboard is shown is to fix a small blank space above the keyboard. This is because of the padding bottom that we apply to the list. If we apply it (includeSafeAreaPaddingBottom) to the ScreenWrapper, the issue won't happen

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

Remove the padding-bottom logic based on the keyboard state from FixedFooter. Then, we will apply the padding-bottom in BaseSelectionList if there is a footer. https://github.com/Expensify/App/blob/807c945ef8edabe86cc62f16c866aa754db8750b/src/components/SelectionList/BaseSelectionList.tsx#L518

(!isKeyboardShown || !!footerContent || showConfirmButton) && safeAreaPaddingBottomStyle

What alternative solutions did you explore? (Optional)

Remove the safe area padding bottom from BaseSelectionList and enable includeSafeAreaPaddingBottom to every page that uses BaseSelectionList. Currently, all pages that use BaseSelectionList disables includeSafeAreaPaddingBottom to not have double padding bottom.

alexpensify commented 1 week ago

On my testing list, I'll get to it soon.

youssef-lr commented 1 week ago

@alexpensify I think this fits as a general improvement to the UI and not particularly tied to any project. It makes the UI looks glitchy and unpolished. Can we get a C+ assigned to review the proposal? Also I think we need to tackle this everywhere in the UI where it happens, and not just the two pages mentioned in this issue.

melvin-bot[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~01419f469e7473b289

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Ollyws (External)

alexpensify commented 1 week ago

Thanks, @youssef-lr, for the context! Assigning the external label now.

@Ollyws please review the proposal and identify if it will fix the issue. Thanks!

Ollyws commented 6 days ago

@bernhardoj's proposal LGTM.

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

melvin-bot[bot] commented 6 days ago

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

melvin-bot[bot] commented 6 days ago

πŸ“£ @Ollyws πŸŽ‰ 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 6 days ago

πŸ“£ @bernhardoj πŸŽ‰ 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 πŸ“–

bernhardoj commented 5 days ago

PR is ready

cc: @Ollyws

youssef-lr commented 5 days ago

@bernhardoj are you seeing this?

image

bernhardoj commented 5 days ago

@youssef-lr yes, I'm seeing that on main too.

melvin-bot[bot] commented 2 days ago

Triggered auto assignment to @kevinksullivan (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.

alexpensify commented 2 days ago

Heads up, I will be offline until Tuesday, May 28, 2024, and will not actively watch over this GitHub during that period.

@kevinksullivan - While I'm offline, I need help here completing the payment process after this one clears the 7-day period. Thanks!