Skip to content
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

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

Open
5 of 6 tasks
IuliiaHerets opened this issue Aug 8, 2024 · 30 comments
Open
5 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 8, 2024

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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6565196_1723085486947.20240808_104712.mp4

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
@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

Copy link

melvin-bot bot commented Aug 8, 2024

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 github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 8, 2024
Copy link
Contributor

github-actions bot commented Aug 8, 2024

👋 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
Copy link
Author

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

@Krishna2323
Copy link
Contributor

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.

<View style={[styles.flex1, styles.pb5]}>

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.

includeSafeAreaPaddingBottom={false}

What alternative solutions did you explore? (Optional)

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

{showConfirmButton && (
<FixedFooter style={[styles.mtAuto]}>
<Button
success
large
style={[styles.w100]}
text={confirmButtonText || translate('common.confirm')}
onPress={onConfirm}
pressOnEnter
enterKeyEventListenerPriority={1}
/>
</FixedFooter>
)}

We need to pass the following props:

                        showConfirmButton
                        confirmButtonText={translate('common.save')}
                        onConfirm={handleConfirmSelection}

@cristipaval cristipaval added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API Hourly KSv2 labels Aug 8, 2024
@cristipaval
Copy link
Contributor

Definitely a NAB. Demoting.

@cristipaval
Copy link
Contributor

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

@cristipaval cristipaval added the External Added to denote the issue can be worked on by a contributor label Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

@melvin-bot melvin-bot bot changed the title Web - Search - Filters - Extra margin below Save button in Category filter [$250] Web - Search - Filters - Extra margin below Save button in Category filter Aug 8, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

@cristipaval cristipaval changed the title [$250] Web - Search - Filters - Extra margin below Save button in Category filter [$125] Web - Search - Filters - Extra margin below Save button in Category filter Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

Upwork job price has been updated to $125

@dominictb
Copy link
Contributor

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:

<View style={[styles.flex1, styles.pb5]}>

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
Copy link
Contributor

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

<View style={[styles.flex1, styles.pb5]}>

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

<Button
success
text={translate('common.save')}
pressOnEnter
onPress={handleConfirmSelection}
large
/>

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

  • For extra bottom padding

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
Copy link

Mohammed-Ehap-Ali-Zean-Al-Abdin commented Aug 8, 2024

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

Copy link

melvin-bot bot commented Aug 8, 2024

📣 @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
Copy link
Contributor

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

@allroundexperts
Copy link
Contributor

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

Copy link

melvin-bot bot commented Aug 11, 2024

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 11, 2024

@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.

function FixedFooter({style, children}: FixedFooterProps) {
const insets = useSafeAreaInsets();
const styles = useThemeStyles();
if (!children) {
return null;
}
const shouldAddBottomPadding = !insets.bottom;
return <View style={[styles.ph5, shouldAddBottomPadding && styles.pb5, styles.flexShrink0, style]}>{children}</View>;
}

const footerContent = useMemo(
() => (
<Button
success
text={translate('common.save')}
pressOnEnter
onPress={handleConfirmSelection}
large
/>
),
[translate, handleConfirmSelection],
);

@allroundexperts
Copy link
Contributor

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

@Krishna2323
Copy link
Contributor

@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.

{!!footerContent && <FixedFooter style={[styles.mtAuto]}>{footerContent}</FixedFooter>}

function FixedFooter({style, children}: FixedFooterProps) {
const insets = useSafeAreaInsets();
const styles = useThemeStyles();
if (!children) {
return null;
}
const shouldAddBottomPadding = !insets.bottom;
return <View style={[styles.ph5, shouldAddBottomPadding && styles.pb5, styles.flexShrink0, style]}>{children}</View>;
}

@Krishna2323
Copy link
Contributor

@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 melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 13, 2024
Copy link

melvin-bot bot commented Aug 13, 2024

📣 @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
Copy link
Contributor

@cristipaval @allroundexperts Quick Nudge on this comment - #47046 (comment)

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 14, 2024
@BhuvaneshPatil
Copy link
Contributor

@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
Copy link
Contributor

bump @allroundexperts on the question above. Thanks!

@allroundexperts
Copy link
Contributor

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

@isabelastisser
Copy link
Contributor

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

@isabelastisser
Copy link
Contributor

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

@BhuvaneshPatil
Copy link
Contributor

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants