[HOLD for payment 2023-10-12] [$500] Address remaining design inconsistencies for distance requests

[JmillsExpensify]

From a post by @shawnborton (cc @dannymcclain)

Changed required:

• Each push-to-page row has margin-bottom: 8px - let’s get rid of that so it’s a bit more compact and matches existing styles
• the map image itself has a bunch of vertical margin (20px) and thus it feels quite far away from the push row below it which also has vertical margin. Let’s make the vertical margin on the map image something like 8px or 12px
• The “To” field above the workspace has a height of 32px. Let’s remove the height and just have some margin-top, but no need for margin below as the workspace row (avatar + name) already has vertical margin. This change should be applied everywhere we have a “To” row
• Let’s make the map preview image use the same border radius as other receipt previews, which I think is 16px

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a22578e8f0e37d00
• Upwork Job ID: 1700221988509253632
• Last Price Increase: 2023-09-15
• Automatic offers:
• situchan | Reviewer | 26849648
• akinwale | Contributor | 26849649

[melvin-bot]

Current assignee @JmillsExpensify is eligible for the NewFeature assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @dannymcclain (Design), see these Stack Overflow questions for more details.

[melvin-bot]

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

[melvin-bot]

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

[JmillsExpensify]

Let’s make the vertical margin on the map image something like 8px or 12px

Coming back to this one, we should decide one way or the other.

[allroundexperts]

@JmillsExpensify I can work on this one and raise a PR over the weekend as I've worked extensively with the DistanceRequests.

[akinwale]

Proposal

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

Design inconsistencies for distance requests.

What is the root cause of that problem?

None. New feature.

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

Each push-to-page row has margin-bottom: 8px - let’s get rid of that so it’s a bit more compact and matches existing styles

Update MoneyRequestConfirmationList and conditionally remove the styles.mb2 style from each displayed MenuItemWithTopDescription if the distance request condition is true.

The map image itself has a bunch of vertical margin (20px) and thus it feels quite far away from the push row below it which also has vertical margin. Let’s make the vertical margin on the map image something like 8px or 12px

Update styles.confirmationListMapItem with the new vertical margin.

The “To” field above the workspace has a height of 32px. Let’s remove the height and just have some margin-top, but no need for margin below as the workspace row (avatar + name) already has vertical margin. This change should be applied everywhere we have a “To” row

Update styles.optionsListSectionHeader with a top margin.

Let’s make the map preview image use the same border radius as other receipt previews, which I think is 16px

Change borderRadius for styles.mapView to 16.

What alternative solutions did you explore? (Optional)

None.

[allroundexperts]

@JmillsExpensify I can work on this one and raise a PR over the weekend as I've worked extensively with the DistanceRequests.

Never mind, I see that @akinwale has a proposal ready!

[situchan]

@akinwale thanks for your proposal. Can you please share demo screenshot after applying your changes?

[akinwale]

@akinwale thanks for your proposal. Can you please share demo screenshot after applying your changes?

@situchan Here are screenshots with my changes applied. EDIT: Added web screenshot.

[akinwale]

@situchan Some additional notes about the margins used. Everything else was updated to follow the original post.

the map image itself has a bunch of vertical margin (20px) and thus it feels quite far away from the push row below it which also has vertical margin. Let’s make the vertical margin on the map image something like 8px or 12px

• Used 8px margin here for both top and bottom.

The “To” field above the workspace has a height of 32px. Let’s remove the height and just have some margin-top, but no need for margin below as the workspace row (avatar + name) already has vertical margin. This change should be applied everywhere we have a “To” row

• Used 8px margin for the top here.

[jeet-dhandha]

Proposal

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

• Distance Request has design inconsistencies like extra spacing between fields and map.

What is the root cause of that problem?

• It was implemented this way only.

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

Solution for 1st and 2nd Points

1. Each push-to-page row has margin-bottom: 8px - let’s get rid of that so it’s a bit more compact and matches existing styles
2 .The map image itself has a bunch of vertical margin (20px) and thus it feels quite far away from the push row below it which also has vertical margin. Let’s make the vertical margin on the map image something like 8px or 12px

• We will update in MoneyRequestConfirmationList with below styles:

1. styles.confirmationListMapItem (spacing.mv3 bcoz we want to update vertical spacing only and as all the items have 24px spacing in between so this keeps the consistency )

- {...spacing.m5, height: 200, }
+ {...spacing.m5, ...spacing.mv3, height: 200, } 

2. Remove each styles.mt2 / styles.mb2 which is used with styles.moneyRequestMenuItem.

Solution for 3rd Point

3. Let’s make the map preview image use the same border radius as other receipt previews, which I think is 16px

• For radius of map referring styles.reportActionItemImages which is 16px.

1. We will update borderRadius in styles.mapView from 20 to 16. (this will update in other places also.)

Solution for 4th Point (I have two solutions)

4. The “To” field above the workspace has a height of 32px. Let’s remove the height and just have some margin-top, but no need for margin below as the workspace row (avatar + name) already has vertical margin. This change should be applied everywhere we have a “To” row

1. Update styles.optionsListSectionHeader to {...spacing.mv1}. (Such that it doesn't cause much UI regression in other places like below). But this will create inconsistency in UI in other places.

Above issue will emerge if we only give marginTop to styles.optionsListSectionHeader

2. Add a overrideHeaderStyle to sections

    sections.push({
        title: translate('common.to'),
        ...rest,
        overrideHeaderStyle: {...spacing.mv1},
    });

and in BaseOptionsList update

- const renderSectionHeader = ({section: {title, shouldShow}}) => {
+ const renderSectionHeader = ({section: {title, shouldShow, overrideHeaderStyle}}) => {
- <View style={[styles.optionsListSectionHeader, styles.justifyContentCenter]}>
+ <View style={[overrideHeaderStyle || styles.optionsListSectionHeader, styles.justifyContentCenter]}> // we can add additional checks if required

What alternative solutions did you explore? (Optional)

• N/A

[shawnborton]

But this will create inconsistency in UI in other places.

Good catch, so maybe we do need to give it at least 4px bottom margin then.

[dannymcclain]

Not overdue. ☝️

[akinwale]

@situchan My PR is ready for review.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @akinwale got assigned: 2023-09-25 04:24:47 Z
• when the PR got merged: 2023-10-03 02:09:41 UTC
• days elapsed: 5

On to the next one 🚀

[melvin-bot]

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

[melvin-bot]

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

• feat: address remaining design inconsistencies for distance requests #28110

If no regressions arise, payment will be issued on 2023-10-12. 🎊

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

For reference, here are some details about the assignees on this issue:

• @akinwale requires payment offer (Contributor)
• @situchan requires payment offer (Reviewer)

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

[JmillsExpensify]

Payment summary:

• Issue reporter: N/A
• Contributor: @akinwale $750
• Contributor+: @situchan $750

[akinwale]

Payment summary:

• Issue reporter: N/A
• Contributor: @akinwale $500
• Contributor+: @situchan $500

There were a couple of days of delays during the PR review, and before the eventual PR merge action. Taking these into account, would this possibly be considered eligible for the urgency bonus? Thanks.

[situchan]

3 business days of delay was from internal review.
Here's proof:

Screen.Recording.2023-10-08.at.8.13.18.PM.mov

[JmillsExpensify]

@joelbettner Can to confirm these more recent comments?

[joelbettner]

@JmillsExpensify Yep, it took me a while to get around to reviewing. I've been completely inundated with auto assigned issues and reviews all the while trying to get higher priority code written for other projects.

[JmillsExpensify]

Payment summary:

• Issue reporter: N/A
• Contributor: @akinwale $750
• Contributor+: @situchan $750

Ok great thanks! I've updated the summary.

[JmillsExpensify]

Still waiting on the BZ checklist from @situchan though in the meantime, I've paid out @akinwale.

[situchan]

This is just design inconsistency, not bug. I don't think regression test applies here.

[melvin-bot]

@JmillsExpensify, @akinwale, @dannymcclain, @joelbettner, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[joelbettner]

@JmillsExpensify I think we can close this issue, yes?

[situchan]

Awaiting payment

[JmillsExpensify]

All paid out and no regression test suggested. Closing.