[$1000] No margin above Remove button in Contact method Detail screen

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to settings -> Profile -> Contact methods
2. Click New contact method -> Add any email which is already on Expensify. (i.e.: [email protected])
3. Click Add button
4. 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?

• [x] Android / native
• [x] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

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

https://user-images.githubusercontent.com/43996225/233814549-eed39140-4288-4aaf-9767-394c4448ad01.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]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[PrashantMangukiya]

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. https://github.com/Expensify/App/blob/4947508bedc2e901082d805c84498f5f394ef655/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js#L193

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

https://user-images.githubusercontent.com/7823358/233819138-2b23600c-bf5e-4dfd-9c2e-3b174d255531.mov

https://user-images.githubusercontent.com/7823358/233819139-7ad5668b-c3e5-4391-9da2-46ed8fda8e8d.mov

[Pujan92]

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. https://github.com/Expensify/App/blob/4947508bedc2e901082d805c84498f5f394ef655/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js#L193

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.

https://github.com/Expensify/App/blob/4947508bedc2e901082d805c84498f5f394ef655/src/pages/settings/Profile/Contacts/ContactMethodsPage.js#L115 https://github.com/Expensify/App/blob/4947508bedc2e901082d805c84498f5f394ef655/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js#L195

Result

https://user-images.githubusercontent.com/14358475/233844554-77e8fada-f686-410a-a22d-4693d56fae8e.mp4

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[hoangzinh]

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 https://github.com/Expensify/App/blob/17c1dd8561f217ed3117937a901425d298196873/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js#L250-L255

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]

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 https://github.com/Expensify/App/blob/d19326ec58085c230eeccbe3e6f4dd9cbef38ae4/src/styles/styles.js#L2669-L2673 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 https://github.com/Expensify/App/blob/d19326ec58085c230eeccbe3e6f4dd9cbef38ae4/src/components/DotIndicatorMessage.js#L55

[johnmlee101]

not overdue still reviewing proposals

[sobitneupane]

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]

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

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

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]

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

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

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

@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 https://github.com/Expensify/App/blob/17c1dd8561f217ed3117937a901425d298196873/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js#L246

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 https://github.com/Expensify/App/issues/17847#issuecomment-1521080256 What do you think?

[sobitneupane]

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

[hoangzinh]

@sobitneupane I agree at the current code. But I still think add margin in Remove button is better place than DotIndicatorMessage:

• It might not true, but if we add new element in this page in future, it won't cause extra space because of margin-bottom of DotIndicatorMessage.
• I think to keep position, the "lower" element should take responsible for it. So if it's not added, there is nothing, but if it's added, it adds space with "upper" element.

[johnmlee101]

Thanks everyone for the discussion. I think I agree with @sobitneupane and will go with @PrashantMangukiya 's solution

[MelvinBot]

📣 @PrashantMangukiya You have been assigned to this job by @johnmlee101! 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 📖

[PrashantMangukiya]

@johnmlee101 @sobitneupane Applied to job on Upwork. I will prepare and submit PR within two hours asap. Thank you.

[PrashantMangukiya]

@sobitneupane @johnmlee101 PR is ready for review.

[kevinksullivan]

Offers sent in upwork @PrashantMangukiya @sobitneupane

[DinalJivani]

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

[melvin-bot[bot]]

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

[sobitneupane]

PR Deployed to Staging

[melvin-bot[bot]]

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