search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.49k stars 2.84k forks source link

[HOLD for payment 2023-06-29] [$1000] No consistency in margin left for green tick besides amount #19953

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

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:

  1. Open the app
  2. Open any report
  3. Send money to report
  4. Click on send money message and using console window, observe margin-left to green tick icon besides amount on top and in message below
  5. Again click on message displayed in that report to sent report
  6. Observe margin left to tick icon in that report

Expected Result:

App should maintain consistent margin between amount and green tick throughout all the reports in the flow

Actual Result:

App maintains different margin for all the green tick instances throughout all the reports in the flow

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.22.0

Reproducible in staging?: Yes

Reproducible in production?: n/a

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://github.com/Expensify/App/assets/93399543/6bcb7d53-e8f2-440c-a680-4510ad425f68

https://github.com/Expensify/App/assets/93399543/00fd7b75-e068-4c55-b11a-d42c5c8feb4b

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685201350349499

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01628393c0c27440ce
  • Upwork Job ID: 1665756974338699264
  • Last Price Increase: 2023-06-12
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

Ollyws commented 1 year ago

Proposal

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

The money request header checkmark and the IOU details checkmark have inconsistent margins.

What is the root cause of that problem?

The marginLeft of iouPreviewBoxCheckmark is set to 4, and moneyRequestHeaderCheckmark is set to 5.

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

We can modify these to be the same value, 8 as requested.

To remove the checkmark from paid IOU threads we can remove the checkmark in ReportPreview.

What alternative solutions did you explore? (Optional)

sonialiap commented 1 year ago

Reproducible

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01628393c0c27440ce

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

kushu7 commented 1 year ago

Proposal

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

No consistency in margin left for green tick besides amount

What is the root cause of that problem?

As stated in Issue. both are using different margin

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

we should remove styles.moneyRequestHeaderCheckmark and use styles.iouPreviewBoxCheckmark as its introduced 2 years back. it will help in maintaining code for same thing. both have same proprties except difference of 1px. We should update MoneyRequestHeader.js and TaskHeader.js to use styles.iouPreviewBoxCheckmark instead of styles.moneyRequestHeaderCheckmark.

What alternative solutions did you explore? (Optional)

None

Pujan92 commented 1 year ago

Proposal

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

Inconsistency between checkmark icon margin with the label

What is the root cause of that problem?

For IOUPreview we have left margin of 4px in the message whereas for the MoneyRequestHeader the margin of 8px(styles.ml2) set within this latest PR.

https://github.com/Expensify/App/blob/71017e42617bedefa04628bef5eb18d1b28df1fb/src/components/ReportActionItem/IOUPreview.js#L203

https://github.com/Expensify/App/blob/71017e42617bedefa04628bef5eb18d1b28df1fb/src/styles/styles.js#L2528-L2529

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

1) We need to update the style to styles.ml1 in the MoneyRequestHeader MenuItem.

https://github.com/Expensify/App/blob/71017e42617bedefa04628bef5eb18d1b28df1fb/src/components/MenuItem.js#L179-L180

2) Also need to update marginLeft to 4px for class moneyRequestHeaderCheckmark. https://github.com/Expensify/App/blob/132358f1e844583cbeb0e5358e54d039f7f47ac0/src/components/MoneyRequestHeader.js#L145 https://github.com/Expensify/App/blob/132358f1e844583cbeb0e5358e54d039f7f47ac0/src/styles/styles.js#L3314-L3315

mollfpr commented 1 year ago

We need to clarify the expected result before selecting the proposal.

Money Request Report Money Request Detail Header Money Request Action
money request report money request header money request report action

@shawnborton Would you help to determine what margin should be set between the amount and checkmark icon from the above places? Thanks!

shawnborton commented 1 year ago

Just to clarify with @trjExpensify and @JmillsExpensify - this screenshot here shouldn't exist right? I thought we fixed this elsewhere: image

Then for the other screens, I think 8px makes sense but we're going to end up redoing the middle screenshot anyways, so I'm not really sure what we should do here.

trjExpensify commented 1 year ago

Whatcha' mean by shouldn't exist? Right now the ReportPreview in the workspace chat looks like that. We haven't made it a "card" style with all those changes just yet, scan receipt is where those are.

shawnborton commented 1 year ago

The report action mockup I shared shouldn't have a green checkmark. The report preview card, yes. But the report action line, no.

trjExpensify commented 1 year ago

Yeah, the report action line in the iou/expenseReport view shouldn't have a checkmark. The report preview (which right now is just in a message style) in the chat did, but given that we're killing it, may as well get rid of it now if you want. 👍

mollfpr commented 1 year ago

@shawnborton @trjExpensify We can change the margin for the first screenshot and remove the checkmark from the report action. Do you think this sounds good to you?

shawnborton commented 1 year ago

That works for me.

grgia commented 1 year ago

@mollfpr do any of these proposals work with the above suggestion? Thank you!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

mollfpr commented 1 year ago

@grgia Not specifically, but closest and first is @Ollyws proposal. We can set the value to 8px and removed the checkmark from the report action.

trjExpensify commented 1 year ago

That works for me.

Same!

Ollyws commented 1 year ago

@mollfpr Does the Money Request Action still have the checkmark in the lastest main for you? Mine doesn't: nocheckmark

Edit: Actually it appears when there's a thread: checkmarkvisible

This can be removed from ReportPreview.

Ollyws commented 1 year ago

Proposal updated.

mollfpr commented 1 year ago

@grgia let's go with @Ollyws proposal which it does what requested before.

🎀 👀 🎀 C+ reviewed!

melvin-bot[bot] commented 1 year ago

@sonialiap @mollfpr @grgia this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 year ago

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

grgia commented 1 year ago

Thanks for your patience, good to go @Ollyws!

melvin-bot[bot] commented 1 year ago

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

On to the next one 🚀

Ollyws commented 1 year ago

Not sure about Melvin's calculations here...it took 4 days, 36 minutes and 4 seconds. Minus the weekend is 2 days and 36 minutes.

sonialiap commented 1 year ago

Thanks for the correction, Olly! Melvin's bad math issue was brought up internally yesterday, I'm not sure if we've created an issue to fix this yet but we're aware and I expect we will fix it 👁️ (Seems to do something with how the API is reading the Z in the timezone)

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-06-29. :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.

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

melvin-bot[bot] commented 1 year ago

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:

mollfpr commented 1 year ago

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR: [@mollfpr] 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:

No offending PR, another improvement on the IOU/money request design.

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

Since this is improvement on the visual, the regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug. [@mollfpr] 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.

Regression step:

  1. Open the app
  2. Open any report
  3. Send money to report
  4. Click on send money message
  5. Verify both the left margin of the checkmark in the header and in the preview box message are a consistent 8px.
  6. Verify that the confirmation text (e.g paid $123.00 ...) does not have a checkmark.
  7. Navigate to IOU Chat.
  8. Verify that the confirmation text (e.g paid $123.00 ...) does not have a checkmark.
  9. 👍 or 👎
sonialiap commented 1 year ago

@dhanashree-sawant offer sent for reporting bug @Ollyws offer sent for fix (+bonus) @mollfpr offer sent for review (+bonus)

June 16 1:10pm (Friday) June 20 11:43am (Tuesday)

Ollyws commented 1 year ago

@sonialiap Accepted, thanks!

dhanashree-sawant commented 1 year ago

@sonialiap Accepted, Thank you!

sonialiap commented 1 year ago

Everyone has been paid ✅