[HOLD for payment 2023-05-16] [$1000] No margin above Remove button in Contact method Detail screen #17847

kavimuru · 2023-04-23T01:33:28Z

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

Action Performed:

Go to settings -> Profile -> Contact methods
Click New contact method -> Add any email which is already on Expensify. (i.e.: test@gmail.com)
Click Add button
Go to detail screen of that same email

Expected Results:

There should be some margin above remove button

Actual Results:

No margin above remove button

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.4
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-04-23.at.3.21.14.AM.mov

Expensify/Expensify Issue URL:
Issue reported by: @DinalJivani
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682202200263469

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0132b1dcfc371624b3
Upwork Job ID: 1650669214496661504
Last Price Increase: 2023-04-25

MelvinBot · 2023-04-23T01:33:34Z

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot · 2023-04-23T01:33:37Z

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

PrashantMangukiya · 2023-04-23T08:07:49Z

Proposal

Posting proposal early as per new guidelines

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

No margin above Remove button in Contact method Detail screen. There should be some margin above remove button where error message text displayed with red dot.

What is the root cause of that problem?

Failed contact method error message displayed via <DotIndicatorMessage> component via this line.

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Line 193 in 4947508

    
           {isFailedAddContactMethod && <DotIndicatorMessage style={[styles.mh5]} messages={ErrorUtils.getLatestErrorField(loginData, 'addedLogin')} type="error" />}

We can see styles.mh5 passed to <DotIndicatorMessage>, but bottom margin style not passed. This is the root cause of the problem.

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

We have to also pass bottom margin style to DotIndicatorMessage. e.g. styles.mb3 i.e. 12 px as shown in code below.

{isFailedAddContactMethod && <DotIndicatorMessage style={[styles.mh5, styles.mb3]} messages={ErrorUtils.getLatestErrorField(loginData, 'addedLogin')} type="error" />}

Note: We have to set bottom margin suggested by design team. At present styles.mb3 set to show code sample. It will solve the issue as shown in results

What alternative solutions did you explore? (Optional)

None

Results

1.mov

2.mov

Pujan92 · 2023-04-23T14:07:51Z

Proposal

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

Vertical margin not available for contact method details DotIndicatorMessage

What is the root cause of that problem?

Vertical margin isn't provided for the DotIndicatorMessage here.

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Line 193 in 4947508

    
           {isFailedAddContactMethod && <DotIndicatorMessage style={[styles.mh5]} messages={ErrorUtils.getLatestErrorField(loginData, 'addedLogin')} type="error" />}

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

Add styles.mv3 to the DotIndicatorMessage, as the same(top margin too required) we are providing to ContactMethods, ContactMethodDetails successful screen.

App/src/pages/settings/Profile/Contacts/ContactMethodsPage.js

Line 115 in 4947508

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Line 195 in 4947508

Result

Screen.Recording.2023-04-23.at.7.35.21.PM.mp4

MelvinBot · 2023-04-25T01:13:06Z

Job added to Upwork: https://www.upwork.com/jobs/~0132b1dcfc371624b3

MelvinBot · 2023-04-25T01:13:10Z

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

MelvinBot · 2023-04-25T01:13:19Z

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

MelvinBot · 2023-04-25T01:13:25Z

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

hoangzinh · 2023-04-25T02:37:36Z

Proposal

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

No margin above Remove button in Contact method Detail screen

What is the root cause of that problem?

Currently, we haven't provide margin style to Remove button in those LOC

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 250 to 255 in 17c1dd8

    
           <OfflineWithFeedback 
        
               pendingAction={lodashGet(loginData, 'pendingFields.deletedLogin', null)} 
        
               errors={ErrorUtils.getLatestErrorField(loginData, 'deletedLogin')} 
        
               errorRowStyles={[styles.mt6]} 
        
               onClose={() => User.clearContactMethodErrors(contactMethod, 'deletedLogin')} 
        
           >

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

We should pass prop style to the wrapper component of Remove button with style={[styles.mv3]} to align with what we set when it's default contact plain text in this line

I think it's better place to set margin.

honnamkuan · 2023-04-25T11:29:51Z

Proposal

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

No vertical margin is specified in places where DotIndicatorMessage is used, resulting in cramped looking UI

What is the root cause of that problem?

Lack of vertical margin styling in DotIndicatorMessage component.

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

We should standardize vertical margin for DotIndicatorMessage, by setting styles.dotIndicatorMessage

App/src/styles/styles.js

Lines 2669 to 2673 in d19326e

    
           dotIndicatorMessage: { 
        
               display: 'flex', 
        
               flexDirection: 'row', 
        
               alignItems: 'center', 
        
           },

to include

...spacing.mv3,

By doing so, we will ensure standardized default vertical margin for all usages of the component, at the same time it can still be overridden when necessary, due to props.style which get passed in will have higher precedence

App/src/components/DotIndicatorMessage.js

Line 55 in d19326e

johnmlee101 · 2023-04-25T19:48:51Z

not overdue still reviewing proposals

sobitneupane · 2023-04-26T05:28:01Z

Thanks for the proposal everyone.

All the proposals are similar. I would suggest to go through the previous proposals and propose your solution only if it has significant difference with the previous proposals.

@PrashantMangukiya has pointed out the root cause and suggested to add some padding in the DotIndicator. So, I would suggest going with his proposal. But let's use vertical padding instead of bottom padding.

🎀👀🎀 C+ reviewed

cc: @johnmlee101

Pujan92 · 2023-04-26T05:34:09Z

@sobitneupane As for this small change the proposals won't be significantly changed and I thought the correct margin in the proposal should be prioritized. Just wanted to make the point.

sobitneupane · 2023-04-26T05:47:04Z

@Pujan92 The proposal pointed out the root cause and the solution was almost up to the point. We cannot reject a proposal for a little lacking. We make some iterations in PR to polish such little things. As you know, it still has not been reviewed by internal engineer and he might think otherwise.

Pujan92 · 2023-04-26T05:52:49Z

Thanks. As the change is a straight forward I thought it better if the proposal covers the connected part.

As you know, it still has not been reviewed by internal engineer and he might think otherwise.

Yes, that's why I raised a point before it got assigned.

PrashantMangukiya · 2023-04-26T05:53:43Z

@Pujan92 and @sobitneupane Thanks.

I also mentioned in my proposal that styles shown for code sample. We will consider style as suggested by design team.

Note: We have to set bottom margin suggested by design team. At present styles.mb3 set to show code sample. It will solve the issue as shown in results

Pujan92 · 2023-04-26T05:59:25Z

@PrashantMangukiya For the other screens which start with the direct Text we are already providing the marginTop of 12px, so as per consistency we should mention this in the proposal which I am seeing missing in your case and I think it might require for this straight forward changes.

PrashantMangukiya · 2023-04-26T06:03:59Z

@PrashantMangukiya For the other screens which start with the direct Text we are already providing the marginTop of 12px, so as per consistency we should mention this in the proposal which I am seeing missing in your case and I think it might require for this straight forward changes.

This is not necessary to mention in proposal. Main thing is to point root cause of the problem and where correction needed.

hoangzinh · 2023-04-26T10:14:17Z

@sobitneupane if we adding margin bottom to DotIndicatorMessage, I think it will add extra margin space in case it's default contact and having error. Because default contact also have margin top

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Line 246 in 17c1dd8

So I think, to be safe, we should stick with the problem and expectation in the GH issue description, that means we should add margin to the Remove button instead as I described here #17847 (comment)
What do you think?

sobitneupane · 2023-04-26T10:40:40Z

@hoangzinh I have considered the case you mentioned. But I don't think the scenario you mentioned is likely to occur. We are adding margin to the error which occurs if adding contact fails which won't occur with already added Default Contact.

PrashantMangukiya · 2023-04-27T18:17:38Z

@sobitneupane @johnmlee101 PR is ready for review.

kevinksullivan · 2023-05-01T16:21:48Z

Offers sent in upwork @PrashantMangukiya @sobitneupane

DinalJivani · 2023-05-01T18:38:02Z

@kevinksullivan
I think you forgot to send offer to me.

melvin-bot · 2023-05-08T20:09:04Z

@johnmlee101, @PrashantMangukiya, @kevinksullivan, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

sobitneupane · 2023-05-09T01:36:13Z

PR Deployed to Staging

melvin-bot · 2023-05-09T03:35:17Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2023-05-09T03:35:18Z

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

Add margin above Remove button in Contact Method Detail screen #18087

If no regressions arise, payment will be issued on 2023-05-16. 🎊

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
Contributor that fixed the issue
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

melvin-bot · 2023-05-09T03:35:20Z

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:

[@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
[@sobitneupane] 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:
[@sobitneupane] 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:
[@sobitneupane] Determine if we should create a regression test for this bug.
[@sobitneupane] 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.
[@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

kevinksullivan · 2023-05-09T19:53:32Z

Offer sent @DinalJivani

kevinksullivan · 2023-05-09T19:53:57Z

@sobitneupane please finish out the steps above this week and we'll get payments out once that is done and the 7 day period has passed!

kevinksullivan · 2023-05-16T01:07:45Z

Hi @sobitneupane checking in on this. Can you finish out the steps? Thanks!

sobitneupane · 2023-05-17T13:58:55Z

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:

[@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#15330

[@sobitneupane] 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:

#15330 (comment)

[@sobitneupane] 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:

This should have been caught during the PR review. I don't think we need to update PR review checklist.

[@sobitneupane] Determine if we should create a regression test for this bug.

I don't think we need any regression test for this bug.

kevinksullivan · 2023-05-18T01:35:18Z

Paying out normal amounts due to 8 days to merge.

kevinksullivan · 2023-05-18T01:37:36Z

Payments sent! Thanks everyone.

PrashantMangukiya · 2023-05-18T01:43:03Z

@kevinksullivan I think we are eligible for the timeline bonus because pr for this issue is merged within three business days. I will request to check the timeline one more.

Issue assigned: Apr 27, 2023, 9:23 PM GMT+5:30
PR merged: Apr 28, 2023, 9:52 PM GMT+5:30

PrashantMangukiya · 2023-05-18T02:11:42Z

@kevinksullivan comment above

kevinksullivan · 2023-05-18T15:45:44Z

@PrashantMangukiya wow, I totally missed the merge date. Sorry about that.

kevinksullivan · 2023-05-18T15:50:05Z

@PrashantMangukiya @sobitneupane sorry again, I've created a new upwork job to pay this out

https://www.upwork.com/jobs/~01db36337e9397423c

Just a heads up I am going to be out of office a few days, so this will get paid early next week. But please comment here once you've accepted the job and I'll try and get this to you asap.

PrashantMangukiya · 2023-05-18T15:50:56Z

@kevinksullivan Bonus offer accepted on Upwork. Thank you.

kevinksullivan · 2023-05-18T16:01:02Z

Love the quick turnaround! Paid and ended contracts. Thanks everyone.

kevinksullivan · 2023-05-18T16:01:10Z

Love the quick turnaround! Paid and ended contracts. Thanks everyone.

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 23, 2023

melvin-bot bot assigned kevinksullivan Apr 23, 2023

kevinksullivan added the External Added to denote the issue can be worked on by a contributor label Apr 25, 2023

melvin-bot bot changed the title ~~No margin above Remove button in Contact method Detail screen~~ [$1000] No margin above Remove button in Contact method Detail screen Apr 25, 2023

melvin-bot bot assigned sobitneupane Apr 25, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 25, 2023

melvin-bot bot assigned johnmlee101 Apr 25, 2023

melvin-bot bot added the Overdue label Apr 25, 2023

melvin-bot bot removed the Overdue label Apr 25, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels May 9, 2023

melvin-bot bot changed the title ~~[$1000] No margin above Remove button in Contact method Detail screen~~ [HOLD for payment 2023-05-16] [$1000] No margin above Remove button in Contact method Detail screen May 9, 2023

melvin-bot bot removed the Reviewing Has a PR in review label May 9, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 15, 2023

sobitneupane mentioned this issue May 17, 2023

New contact method #15330

Merged

54 tasks

kevinksullivan closed this as completed May 18, 2023

[HOLD for payment 2023-05-16] [$1000] No margin above Remove button in Contact method Detail screen #17847

[HOLD for payment 2023-05-16] [$1000] No margin above Remove button in Contact method Detail screen #17847

Comments

kavimuru commented Apr 23, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Results:

Actual Results:

Workaround:

Platforms:

MelvinBot commented Apr 23, 2023

MelvinBot commented Apr 23, 2023 • edited by kevinksullivan Loading

Bug0 Triage Checklist (Main S/O)

PrashantMangukiya commented Apr 23, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Pujan92 commented Apr 23, 2023

Proposal

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

What is the root cause of that problem?

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

MelvinBot commented Apr 25, 2023

MelvinBot commented Apr 25, 2023

MelvinBot commented Apr 25, 2023

MelvinBot commented Apr 25, 2023

hoangzinh commented Apr 25, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

honnamkuan commented Apr 25, 2023

Proposal

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

What is the root cause of that problem?

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

johnmlee101 commented Apr 25, 2023

sobitneupane commented Apr 26, 2023

Pujan92 commented Apr 26, 2023 • edited Loading

sobitneupane commented Apr 26, 2023

Pujan92 commented Apr 26, 2023

PrashantMangukiya commented Apr 26, 2023

Pujan92 commented Apr 26, 2023

PrashantMangukiya commented Apr 26, 2023

hoangzinh commented Apr 26, 2023

sobitneupane commented Apr 26, 2023 • edited Loading

PrashantMangukiya commented Apr 27, 2023

kevinksullivan commented May 1, 2023

DinalJivani commented May 1, 2023

melvin-bot bot commented May 8, 2023

sobitneupane commented May 9, 2023

melvin-bot bot commented May 9, 2023

melvin-bot bot commented May 9, 2023 • edited by kevinksullivan Loading

melvin-bot bot commented May 9, 2023

kevinksullivan commented May 9, 2023

kevinksullivan commented May 9, 2023

kevinksullivan commented May 16, 2023

sobitneupane commented May 17, 2023

kevinksullivan commented May 18, 2023

kevinksullivan commented May 18, 2023

PrashantMangukiya commented May 18, 2023

PrashantMangukiya commented May 18, 2023

kevinksullivan commented May 18, 2023

kevinksullivan commented May 18, 2023

PrashantMangukiya commented May 18, 2023

kevinksullivan commented May 18, 2023

kevinksullivan commented May 18, 2023

kavimuru commented Apr 23, 2023 •

edited by melvin-bot bot

Loading

MelvinBot commented Apr 23, 2023 •

edited by kevinksullivan

Loading

hoangzinh commented Apr 25, 2023 •

edited

Loading

Pujan92 commented Apr 26, 2023 •

edited

Loading

sobitneupane commented Apr 26, 2023 •

edited

Loading

melvin-bot bot commented May 9, 2023 •

edited by kevinksullivan

Loading