[HOLD for payment 2023-04-28] [$1000] Feature request : add some bottom padding to the copy link bottom-docked dialog on mobile

[kavimuru]

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

---

Problem:

Incorrect bottom padding in the copy URL bottom-docked modal

Solution:

Increase the bottom padding to match the top padding.

Context/Examples/Screenshots/Notes:

Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL: Issue reported by: @puneetlath Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681313186606359

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b61ef9ad2019e632
• Upwork Job ID: 1646530758878363648
• Last Price Increase: 2023-04-13

[MelvinBot]

Triggered auto assignment to @trjExpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[trjExpensify]

@puneetlath @shawnborton can we firm up the OP on this one a bit before I move it along to guide the proposal? How much bottom padding do we want to add etc?

[shawnborton]

It should match the same padding that is on the top... so that might be either like 12px or 16px?

[MelvinBot]

Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[trjExpensify]

Thanks! Updated. Adding bug and external

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01b61ef9ad2019e632

[MelvinBot]

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

[MelvinBot]

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

[aman-atg]

Proposal

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

Add some bottom padding to the copy link bottom-docked dialog on mobile

What is the root cause of that problem?

This is a feature request.

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

To maintain consistency we should use the same padding on bottom (12px) here for CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED modal

[MelvinBot]

Triggered auto assignment to @alex-mechler (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[daraksha-dk]

Proposal

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

Feature request : add some bottom padding to the copy link bottom-docked dialog on mobile

What is the root cause of that problem?

This is required because we didn't define any padding-bottom for "bottom-docked" modal in mobile

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

We just need to add paddingBottom: 12 here

https://github.com/Expensify/App/blob/3abe7b2f9e38dba42edf406ca1b1e19087f5599d/src/styles/getModalStyles/getBaseModalStyles.js#L135-L142

Note:- The solution is for the bottom-docked modal and not just specific to the scenario when the only item is "Copy URL to Clipboard". It's only more noticeable here because it also has descriptionText as URL (eg. https://test.com) but it should be fix in all scenarios.

[IlyaApaniuk]

Proposal

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

Feature request : add some bottom padding to the copy link bottom-docked dialog on mobile

What is the root cause of that problem?

paddingTop: 12 rule

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

remove paddingTop: 12

[MelvinBot]

📣 @IlyaApaniuk! 📣

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. 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.
2. 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.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[IlyaApaniuk]

Contributor details Your Expensify account email: ilya21968@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01517205235949d8f7

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[ahmedGaber93]

Proposal

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

add some bottom padding to the copy link bottom-docked dialog on mobile

What is the root cause of that problem?

the problem is we depond on safeAreView to set padding bottom in modal container view. and this work fine if iphone have home bar notch, but in devices dos't have home bar notch there is no bottom padding is set in modal container view.

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

if no safeAreaView padding bottom we need to add it here by on of the two soulation:

const modalPaddingStyles = StyleUtils.getModalPaddingStyles({
    ...
    // first soluation
    safeAreaPaddingBottom: insets.bottom === 0 ? 12 : safeAreaPaddingBottom,
    ...
    // second soluation
    modalContainerStyleMarginBottom: insets.bottom === 0 ? 12 : modalContainerStyle.marginBottom,
    ...
});

What alternative solutions did you explore? (Optional)

[dianaferreira1]

Proposal

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

To add some bottom padding to the copy link bottom-docked dialog on mobile

What is the root cause of that problem?

Padding-bottom for "bottom-docked" modal in mobile is not defined in code!

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

To add bottom padding to the copy link bottom-docked dialog on mobile, you can add the following style to the modalContainerStyle object inside the "CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED" switch case: modalContainerStyle = { width: '100%', borderTopLeftRadius: 20, borderTopRightRadius: 20, paddingTop: 12, paddingBottom: 16, // add bottom padding justifyContent: 'center', overflow: 'hidden', };

What alternative solutions did you explore?

Alternative to add some bottom padding to the BOTTOM_DOCKED modal type, we can modify the modalContainerStyle object for this case in the switch statement. We can add a paddingBottom property with the desired value of padding (for example, 16):

case CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED: modalStyle = { ...modalStyle, ...{ alignItems: 'center', justifyContent: 'flex-end', }, }; modalContainerStyle = { width: '100%', borderTopLeftRadius: 20, borderTopRightRadius: 20, paddingTop: 12, justifyContent: 'center', overflow: 'hidden', paddingBottom: 16, // add bottom padding here };

shouldAddBottomSafeAreaPadding = true;
swipeDirection = undefined;
animationIn = 'slideInUp';
animationOut = 'slideOutDown';
break;

This will add a 16 point bottom padding to the BOTTOM_DOCKED modal type. You can adjust the value to whatever padding you need.

[thesahindia]

@shawnborton, this will not be specific to only copy link. The bottom padding will be applied to all the bottom docked modals e.g. emoji picker, confirmation modal. All modals docked to the bottom have a top padding of 12 pixels so I believe it's fine to add 12px bottom padding? Can you please confirm it?

[shawnborton]

Yup, confirmed. Let's make this change global to all bottom-docked modals.

[thesahindia]

~@alex-mechler, I like @aman-atg's proposal.~

~C+ reviewed 🎀👀🎀~

[ahmedGaber93]

@thesahindia I think we need to add this margin only in screens dosn't have home bar like the second image but first image with home bar don't need extra margin, and if we add to it, it will be shown inconsistent.

[thesahindia]

Ahh, that makes sense!

@alex-mechler, let's assign @ahmedGaber93. Their proposal is better.

C+ reviewed 🎀👀🎀

[alex-mechler]

Their proposal looks good to me as well. Good catch about the safe area view impacting this as well. Assigning them!

[MelvinBot]

📣 @ahmedGaber93 You have been assigned to this job by @alex-mechler! Please apply to this job in Upwork 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 📖

[ahmedGaber93]

@alex-mechler @thesahindia PR is ready for review.

[MelvinBot]

@trjExpensify, @alex-mechler, @ahmedGaber93, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick!

[thesahindia]

Not overdue! PR is in production. Not sure why the title wasn't updated.

[alex-mechler]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.3-2 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/17495

If no regressions arise, payment will be issued on 2023-04-28. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [ ] External issue reporter
• [x] Contributor that fixed the issue
• [x] Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[alex-mechler]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@thesahindia] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@thesahindia] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [ ] [@thesahindia] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [ ] [@trjExpensify] Determine if we should create a regression test for this bug.
• [ ] [@thesahindia] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [ ] [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[alex-mechler]

Not sure why automation failed, thats the comments that should have been posted though!

[thesahindia]

It wasn't a regression. It was a design choice. Also I think we don't have to add any regression test steps for this.

[trjExpensify]

Agreed. I think this would be classed as a visual bug, so we wouldn't have a regression test case written up for it.

I've sent the below offers. No #urgency bonus applies here because the issue was assigned on the 14th and approved/merged on the 19th. Let me know if you disagree with that though:

• $1,000 to @thesahindia for C+ review
• $1,000 to @ahmedGaber93 for the fix

[ahmedGaber93]

@trjExpensify

the issue was assigned on the 14th and approved/merged on the 19th.

The dates mentioned are correct, but this period includes 2 days weekend, so I think the PR merged within 3 business days

[alex-mechler]

Yup, the 15th and the 16th were weekends, so that would be exactly 3 business days.

[trjExpensify]

Hm, why are we discounting Friday as a business day, when the issue was assigned on that day? Assigned on the 14th at 7:29 UTC, merged on the 19th at 7:31 UTC.

[alex-mechler]

😕 not sure why I miscounted there, but yeah, that is correct. That would be just over the 3 day mark

[thesahindia]

Accepted the offer. Thanks!

[trjExpensify]

Settled up with you both, closing!