Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD FOR PAYMENT JUNE 21][$250] "Uploading attachment" text too close to message #42297

Closed
2 of 6 tasks
m-natarajan opened this issue May 16, 2024 · 20 comments
Closed
2 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented May 16, 2024

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


Version Number: 1.4.74-1
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
Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1715818269205899

Action Performed:

  1. Open any report
  2. Send a image with text

Expected Result:

There should be a small gap between the text you entered and the "Uploading attachment..." text

Actual Result:

There is no gap between them

Workaround:

unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
CleanShot 2024-05-15 at 20 09 25@2x

Snip - (147) New Expensify - Google Chrome

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f281695ab48cd898
  • Upwork Job ID: 1791158555071238144
  • Last Price Increase: 2024-05-16
Issue OwnerCurrent Issue Owner: @thesahindia
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 16, 2024
Copy link

melvin-bot bot commented May 16, 2024

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@dragnoir
Copy link
Contributor

dragnoir commented May 16, 2024

Proposal

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

Expense - Color gradient of the background image in expense report is broken

What is the root cause of that problem?

Missing margin for the component rendering the "Uploading attachment.."

When we upload an image with a text, there's an optimistic comment that will be saved locally and displayed on the report as TEXT+"Uploading attachment.." with a line break in between

htmlForNewComment = `${commentText}\n${CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML}`;

This htmlForNewComment doe snot dupport margin between the TEXT and the CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML

What alternative solutions did you explore?

Add margin to the CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML

htmlForNewComment = `${commentText}\n${CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML}`;

We can engloba the CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML with a HTML element that support margin

1- Add a new customHTMLElementModels here

'uploading-attachment': HTMLElementModel.fromCustomModel({
    tagName: 'uploading-attachment',
    mixedUAStyles: {...styles.mt3},
    contentModel: HTMLContentModel.block,
}),

update here to:

htmlForNewComment = `${commentText}\n<uploading-attachment>${CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML}</uploading-attachment>`;

POC:

20240516_154002.mp4

@slafortune slafortune added the External Added to denote the issue can be worked on by a contributor label May 16, 2024
@melvin-bot melvin-bot bot changed the title "Uploading attachment" text too close to message [$250] "Uploading attachment" text too close to message May 16, 2024
Copy link

melvin-bot bot commented May 16, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 16, 2024
Copy link

melvin-bot bot commented May 16, 2024

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

@nkdengineer
Copy link
Contributor

Proposal

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

There is no gap between them

What is the root cause of that problem?

  • When sending a message with text, we use this optimistic message text.

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

  • We can add an additional \n character to the above to make a gap.

What alternative solutions did you explore? (Optional)

  • NA

@thesahindia
Copy link
Member

@dragnoir's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented May 19, 2024

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

@manishram
Copy link

Proposal

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

In the issue, it is mentioned that the "Uploading attachment" text which shows when we actually add any attachment while sending a message shows very close to the actual message. This makes "Uploading attachment" text indistinguishable from the actual message. And hence making a bad UI.

What is the root cause of that problem?

htmlForNewComment = `${commentText}\n${CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML}`;

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

  • Instead of just making space, I think we should also change the color of the text a little different from the font color of the actual message. This can be a little bit greyish. At the same time, this text should also animate fade-in - fade-out slowly to give the user a clear idea that "Uploading attachment" is a system information that will stay there until the attachment uploads successfully.
issue-42297.mp4

What alternative solutions did you explore? (Optional)

NA

@melvin-bot melvin-bot bot added the Overdue label May 21, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label May 22, 2024
Copy link

melvin-bot bot commented May 22, 2024

📣 @dragnoir 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 melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Overdue Daily KSv2 labels May 22, 2024
@slafortune
Copy link
Contributor

There is a PR for review - I'll be on vacation until June 4th, so adding a BZ in the meantime. I'll take this back on the 4th if needed.

@slafortune slafortune removed the Bug Something is broken. Auto assigns a BugZero manager. label May 23, 2024
@slafortune slafortune removed their assignment May 23, 2024
@slafortune slafortune added the Bug Something is broken. Auto assigns a BugZero manager. label May 23, 2024
Copy link

melvin-bot bot commented May 23, 2024

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 23, 2024
@slafortune slafortune self-assigned this May 24, 2024
@slafortune slafortune added Weekly KSv2 and removed Daily KSv2 labels May 24, 2024
@twisterdotcom twisterdotcom removed their assignment Jun 12, 2024
@mallenexpensify
Copy link
Contributor

PR merged today
#42516

@mallenexpensify mallenexpensify removed the Reviewing Has a PR in review label Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

@slafortune @dragnoir @mallenexpensify @grgia @thesahindia this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@slafortune
Copy link
Contributor

Payment due June 19th with no regressions.

@slafortune slafortune changed the title [$250] "Uploading attachment" text too close to message [HOLD FOR PAYMENT JUNE 19][$250] "Uploading attachment" text too close to message Jun 13, 2024
@mallenexpensify mallenexpensify added the Awaiting Payment Auto-added when associated PR is deployed to production label Jun 18, 2024
@mallenexpensify mallenexpensify changed the title [HOLD FOR PAYMENT JUNE 19][$250] "Uploading attachment" text too close to message [HOLD FOR PAYMENT JUNE 21][$250] "Uploading attachment" text too close to message Jun 18, 2024
@mallenexpensify
Copy link
Contributor

@slafortune , looks like it should be paid on the 21st, based on PR hitting production on the 14th. updated the title to reflect.

@slafortune
Copy link
Contributor

OHHH! Thanks @mallenexpensify, my mistake 👍

@slafortune
Copy link
Contributor

slafortune commented Jun 21, 2024

@dragnoir offer sent here - https://www.upwork.com/nx/wm/offer/102829200 - Paid ✅
@thesahindia as C+ is due $250 via NewDot

@melvin-bot melvin-bot bot added the Overdue label Jun 21, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jun 21, 2024
@kidroca
Copy link
Contributor

kidroca commented Jul 5, 2024

I'm a bit unsure about the need for a custom <uploading-attachment> element to fix this issue. Does it serve any purpose beyond addressing styling problems? In other cases, we use <br /> to handle new line gaps. Could we have used <br /> elements for the optimistic content as well?

I'm working on replacing the "Uploading attachment..." text with the actual attachment. Should I keep the <uploading-attachment> custom element, or is it okay to remove it once the text is replaced with a preview of the attachment?

@shawnborton
Copy link
Contributor

@kidroca I think you make some good points. I am cool with whatever solution you think is best, as long as we achieve the desired design/style where there is a clear space between the message and the image below it. So if you want to undo the custom element, that is fine by me.

@JmillsExpensify
Copy link

$250 approved for @thesahindia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
Development

No branches or pull requests