[$250] Web - Request money- Copy icon on Scan page is displayed below the text

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:

Open website on Android
Click on + > Request money > Scan

Expected Result:

Copy icon on Scan page is displayed in the same line with text

Actual Result:

Copy icon on Scan page is displayed below the text

Workaround:

Unknown

Platforms:

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

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

Version Number: 1.3.62.0 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

Screenshot_20230903_110725_Chrome

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01230e5b7ac07d0a5e
Upwork Job ID: 1698836140237864960
Last Price Increase: 2023-09-04
Automatic offers:

jjcoffee | Reviewer | 26607219
mahmudjontoraqulov | Reporter | 26607222

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @adelekennedy (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)

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

rmm-fl commented 1 year ago

Proposal

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

Copy icon on Scan page is displayed below the text

What is the root cause of that problem?

https://github.com/Expensify/App/blob/52b1e3e9abaabdaf79c0eeafef5665292cf2ab9c/src/pages/iou/ReceiptSelector/index.js#L104-L111

https://github.com/Expensify/App/blob/52b1e3e9abaabdaf79c0eeafef5665292cf2ab9c/src/components/CopyTextToClipboard.js#L28-L37

https://github.com/Expensify/App/blob/52b1e3e9abaabdaf79c0eeafef5665292cf2ab9c/src/components/Pressable/PressableWithDelayToggle.js#L101-L113

When inline is true for PressableWithDelayToggle (which is the default), the text & icon both will be in a component that uses display: inline so that results in text & icon wrapping in certain screen widths, just like normal inline text.

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

Disable inline for PressableWithDelayToggle inside the CopyTextToClipboard component. Which will put the text & icon inside a display:flex; flex-direction:row; component thanks to the implementation in PressableWithDelayToggle. And that will result in them being coupled and not wrap in certain conditions.

What alternative solutions did you explore? (Optional)

Instead of disabling inline for the general component CopyTextToClipboard, we could just expose the inline prop from CopyTextToClipboard with similar default value for inline as the PressableWithDelayToggle and then disable inline just for the ReceiptSelector page.

melvin-bot[bot] commented 1 year ago

📣 @rmm-fl! 📣 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:

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

rmm-fl commented 1 year ago

Contributor details Your Expensify account email: rajesh0malviya@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~016d9a497e49cabeb4

melvin-bot[bot] commented 1 year ago

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

neonbhai commented 1 year ago

Proposal

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

[Distance] - Copy icon on Scan page is displayed below the text

What is the root cause of that problem?

We don't have a parent View componet with CSS to keep both the email and the Copy component glued together The email and icon is rendered by CopyTextToClipBoard which is in turn rendered by the PressableWithDelayToggle component, that lets the icon wrap around the label text.

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

We should add a prop called shouldRenderOnSameLine to pressableWithDelayToggle component that makes it render the icon in the same line.

We will be wrapping the both the the components here with a <View>

The wrapping View should have flex styling and alignItems: 'row', so that they don't wrap around.

This makes sure the user knows the copy button is for the email. We can turn on this prop for places where a CopyTextToClipBoard component is using it.

What alternative solutions did you explore? (Optional)

xx

hungvu193 commented 1 year ago

Proposal

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

[Distance] - Copy icon on Scan page is displayed below the text

What is the root cause of that problem?

The props inline of PressableWithDelayToggle default is true make the 2 views wrapped inside a Text without alignment which caused the issue.

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

To make it render in one line, we can just add style style={[props.inline ? styles.dInlineFlex : {}]} in our PressableView here or just style={[props.inline ? styles.dFlex : {}]}. We shouldn't wrap the content of PressableView with a View due to RN limitation that already wrote here.

What alternative solutions did you explore? (Optional)

N/A N/A

adelekennedy commented 1 year ago

reproducible

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01230e5b7ac07d0a5e

melvin-bot[bot] commented 1 year ago

Current assignee @adelekennedy 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 - @jjcoffee (External)

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $250

jjcoffee commented 1 year ago

Reviewing tomorrow!

jjcoffee commented 1 year ago

Unable to reproduce on latest staging (v1.3.64-2). Can anyone else?

rmm-fl commented 1 year ago

@jjcoffee, I can reproduce it in latest staging

https://github.com/Expensify/App/assets/143930462/1ee82393-3b91-4eec-85c4-0f2ff7858361

jjcoffee commented 1 year ago

@rmm-fl I don't think we design for <320px since almost no devices have that screen size. I imagine the padding has been adjusted since the original issue was created, which probably sorted the issue!

rmm-fl commented 1 year ago

I found another case where it triggers - for width ranging from 595 to 610 I think it's better to couple both the email & icon anyway.

(which is what my proposal proposes, make the inline prop for PressableWithDelayToggle to false)

jjcoffee commented 1 year ago

Sweet, good catch! It's a bit of an unusual device size but probably still worth fixing as it will be affecting other screens too (see below).

I'm happy with @rmm-fl's proposal, the RCA is correct and the solution makes sense as it will fix the same issue on other screens, such as the contact methods page:

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 1 year ago

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

rmm-fl commented 1 year ago

Thanks for the reviewing the proposal @jjcoffee.

(Unrelated: How do I get access to slack & stackoverflow servers? I did send two emails to contributors@expensify.com, 3 days ago and another 2 hours ago, no updates yet!)

jjcoffee commented 1 year ago

@rmm-fl Stackoverflow is internal only. If you've emailed, you should get access to Slack eventually, there might just be a delay at the moment as there's the React Native EU conference on. Not sure if you have any more context @adelekennedy or even the special powers to get @rmm-fl access to Slack?

rmm-fl commented 1 year ago

~~(I notice that there is merge freeze going on, I would prefer to hold the assignment till it's open, in case the potentially created PR wouldn't get merged in 3 days)~~

Merge freeze has ended

MonilBhavsar commented 1 year ago

Looks good to me too! cc @Expensify/design

melvin-bot[bot] commented 1 year ago

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 year ago

📣 @rmm-fl You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 1 year ago

📣 @mahmudjontoraqulov 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

rmm-fl commented 1 year ago

I have applied & PR is here https://github.com/Expensify/App/pull/27136

jjcoffee commented 1 year ago

So @rmm-fl has run into an issue on the PR where on native platforms the copy text appears misaligned with the rest of the text like this:

@rmm-fl doesn't seem to think this is easily fixable, so I think we either need to re-open this for other proposals or just close since it's a fairly minor issue that isn't affecting usability (and only happens at pretty specific screen sizes). cc @adelekennedy

jjcoffee commented 1 year ago

@adelekennedy Can you re-apply the Help Wanted label and switch back to Daily for this? Unless you think this should be closed. (see my comment above).

adelekennedy commented 1 year ago

oof - it's so small and rare that I don't think it's worth the money to fix @MonilBhavsar for a second opinion

jjcoffee commented 1 year ago

@adelekennedy We could also just apply the fix on web and leave it for native, since the PR is already up and running? (That is if @rmm-fl is still interested!)

MonilBhavsar commented 1 year ago

Sorry, I missed this ping. We also updated Scan page screen and we no longer have this copy. So we can close this @adelekennedy

Expensify / App