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.53k stars 2.88k forks source link

[$125] Web - Search - Filters - Extra margin below Save button in Category filter #47046

Open IuliiaHerets opened 3 months ago

IuliiaHerets commented 3 months 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: v9.0.18-1 Reproducible in staging?: Y Reproducible in production?: N Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com

Action Performed:

  1. Go to staging.new.expensify.com/search/filters
  2. Click Category.

Expected Result:

There will be no extra margin below the Save button (production behavior).

Actual Result:

There is extra margin below the Save button.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/b8db6fe4-fcfb-43a8-b2d1-2a53320d095d

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012f06d1723cda4320
  • Upwork Job ID: 1821488943174214137
  • Last Price Increase: 2024-08-08
  • Automatic offers:
    • dominictb | Contributor | 103505005
Issue OwnerCurrent Issue Owner: @allroundexperts
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @cristipaval (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] commented 3 months ago

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

github-actions[bot] commented 3 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
IuliiaHerets commented 3 months ago

We think that this bug might be related to #wave-collect - Release 1

Krishna2323 commented 3 months ago

Proposal

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

Web - Search - Filters - Extra margin below Save button in Category filter

What is the root cause of that problem?

Extra padding bottom is applied. When we pass footer component them it is wrapped with FixedFooter which already applies the margin for the footerContent.

https://github.com/Expensify/App/blob/53eee53ac8bfbdfd4d8eeaf16940e3b165483373/src/pages/Search/SearchFiltersCategoryPage.tsx#L138

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

Remove styles.pb5.

We should also remove includeSafeAreaPaddingBottom={false} if needed, the default is true so we can remove that. https://github.com/Expensify/App/blob/53eee53ac8bfbdfd4d8eeaf16940e3b165483373/src/pages/Search/SearchFiltersCategoryPage.tsx#L129

What alternative solutions did you explore? (Optional)

We can use the confirm button in SelectionList and the we can remove footer content from SearchFiltersCategoryPage.

https://github.com/Expensify/App/blob/53eee53ac8bfbdfd4d8eeaf16940e3b165483373/src/components/SelectionList/BaseSelectionList.tsx#L726-L738

We need to pass the following props:

                        showConfirmButton
                        confirmButtonText={translate('common.save')}
                        onConfirm={handleConfirmSelection}
cristipaval commented 3 months ago

Definitely a NAB. Demoting.

cristipaval commented 3 months ago

I'm also making it a $125 issue given the low complexity of the issue.

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~012f06d1723cda4320

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

Upwork job price has been updated to $125

dominictb commented 3 months ago

Proposal

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

There is extra margin below the Save button.

What is the root cause of that problem?

We add pd5 to add more space in IOS device in PR, but does not consider that we do not need to add it in other platform: https://github.com/Expensify/App/blob/53eee53ac8bfbdfd4d8eeaf16940e3b165483373/src/pages/Search/SearchFiltersCategoryPage.tsx#L138

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

We can get:

    const safePaddingBottomStyle = useSafePaddingBottomStyle();

and use it instead of styles.pb5

BhuvaneshPatil commented 3 months ago

Proposal

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

Web - Search - Filters - Extra margin below Save button in Category filter

There is one more bug on same page - Save button doesn't have top padding while we give padding on other pages.

Category page - no top padding Screenshot 2024-08-08 at 8 21 03 PM
Currency page - top padding present Screenshot 2024-08-08 at 8 19 57 PM

What is the root cause of that problem?

For original issue - Extra bottom padding That is being added with pb5 https://github.com/Expensify/App/blob/cc4626f393daf4171a0bccc190be39132ac021ea/src/pages/Search/SearchFiltersCategoryPage.tsx#L138

For issue related to top padding -> we don't apply top padding to button

https://github.com/Expensify/App/blob/cc4626f393daf4171a0bccc190be39132ac021ea/src/pages/Search/SearchFiltersCategoryPage.tsx#L114-L120

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

We can change pb5 of styles with result of useSafePaddingBottomStyle(). PS - I tried removing it and it worked but as mentioned in other proposal this is for IOS devices.

For adding top padding - apply pt5 (can be different as well, based on internal engineer opinion) style to button.

Result -

Screenshot 2024-08-08 at 8 31 16 PM

What alternative solutions did you explore? (Optional)

Mohammed-Ehap-Ali-Zean-Al-Abdin commented 3 months ago

Proposal

By change padding bottom of this div and make it 0px (the parent div that contains save button and categories names):

Screenshot 2024-08-08 181751

Add padding top by 20px for this div (the parent div of the parent div of the save button):

Screenshot 2024-08-08 182052

This is the result:

Screenshot 2024-08-08 174108

Contributor details Your Expensify account email: mohammed.ehap.zean@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/mohammede132?mp_source=share

melvin-bot[bot] commented 3 months ago

📣 @Mohammed-Ehap-Ali-Zean-Al-Abdin! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
isabelastisser commented 3 months ago

Hey @allroundexperts, can you please review the proposals above? Thanks!

allroundexperts commented 2 months ago

Thanks for the proposals everyone. I think @dominictb's proposal has the correct RCA and the fix makes sense as well. Let's go with them.

@BhuvaneshPatil You're trying to solve another bug here which isn't related to this issue. However, given the simplicity of this, I think we can merge the both into a single bug and pay $125 each to @BhuvaneshPatil and @dominictb. @cristipaval Let me know what you think.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

Current assignee @cristipaval is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

Krishna2323 commented 2 months ago

@allroundexperts, what do you think about my alternative solution? FixedFooter is already covering the case padding bottom case and we don't need to create the footerContent since it can be created using the props.

https://github.com/Expensify/App/blob/5dd0d8b278b0e68231b0e332060d7ae44a3d034e/src/components/FixedFooter.tsx#L16-L27

https://github.com/Expensify/App/blob/53eee53ac8bfbdfd4d8eeaf16940e3b165483373/src/pages/Search/SearchFiltersCategoryPage.tsx#L112-L123

allroundexperts commented 2 months ago

@Krishna2323 I'd much rather go with a more simpler solution 😄

Krishna2323 commented 2 months ago

@allroundexperts, I believe we can simply remove the padding top because the footerContent is already wrapped in FixedFooter and fixed footer already covers the padding for devices with no padding bottom. Please give me few moments and I will confirm.

https://github.com/Expensify/App/blob/5dd0d8b278b0e68231b0e332060d7ae44a3d034e/src/components/SelectionList/BaseSelectionList.tsx#L739

https://github.com/Expensify/App/blob/5dd0d8b278b0e68231b0e332060d7ae44a3d034e/src/components/FixedFooter.tsx#L16-L27

Krishna2323 commented 2 months ago

@allroundexperts, the padding bottom is already covered in FixedFooter. We don't need to add extra padding bottom.

New chat page Category selector after removing padding bottom from view
Monosnap iPhone 15 Plus 2024-08-12 04-51-07 Monosnap iPhone 15 Plus 2024-08-12 04-51-24
melvin-bot[bot] commented 2 months ago

📣 @dominictb 🎉 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 📖

BhuvaneshPatil commented 2 months ago

@cristipaval @allroundexperts Quick Nudge on this comment - https://github.com/Expensify/App/issues/47046#issuecomment-2282907045

BhuvaneshPatil commented 2 months ago

@allroundexperts @cristipaval do we want to cover top padding in this issue as well? Because it's still not fixed

Screenshot 2024-08-19 at 9 01 45 PM
isabelastisser commented 2 months ago

bump @allroundexperts on the question above. Thanks!

allroundexperts commented 2 months ago

@isabelastisser This is more of a question for you / @cristipaval.

isabelastisser commented 2 months ago

@BhuvaneshPatil In my opinion, we should fix that, too. @cristipaval, do you agree? Thanks!

isabelastisser commented 1 month ago

@allroundexperts, Cristi is on leave. Let's go ahead and fix this, thanks!

BhuvaneshPatil commented 1 month ago

@allroundexperts Do you want me to raise the PR or current assigned contributor will do that? If the automation and all allows, I am happy to raise the PR.

allroundexperts commented 1 month ago

@BhuvaneshPatil You can do that since you were the reporter 😄

dominictb commented 1 month ago

In my opinion, we should fix that, too. @cristipaval, do you agree? Thanks!

@isabelastisser I don't believe we should expand the scope of this issue. The main objective is to fix 'There is extra margin below the Save button,' which is why the issue is priced at just $125. If we want to fix the additional bug you mentioned, we should create a separate issue so other contributors can focus on providing a suitable solution.

isabelastisser commented 1 month ago

@allroundexperts, how can we move this issue forward?

allroundexperts commented 1 month ago

@isabelastisser It's better to create a new issue. Can you please create one and assign me and @BhuvaneshPatil? Thanks!

BhuvaneshPatil commented 3 weeks ago

@isabelastisser ^^

melvin-bot[bot] commented 1 week ago

This issue has not been updated in over 15 days. @cristipaval, @allroundexperts, @isabelastisser, @dominictb eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!