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.64k stars 2.93k forks source link

[Pay 29/4] [$250] HIGH: [Polish] Combine image attachment with the unsent composed message #35977

Closed quinthar closed 7 months ago

quinthar commented 10 months ago

Problem:

Today when you upload an image while a message is drafted in the compose window, it:

  1. Sends whatever is drafted
  2. Sends a 2nd message containing only the image

This means that even if the two are conceptually related (ie, the comment is describing the image), it will be forked into separate reactions, and have separate threads.

Solution

Rather than sending images as a totally separate message, combine the message and the attachment into a single message, such that it gets the same reactions and are part of the same thread.

Issue OwnerCurrent Issue Owner: @bfitzexpensify
roryabraham commented 10 months ago

Personally I think this issue kind of "beats around the bush" – I would love to see our efforts spent towards building full support for inline images in messages, GitHub-style, with image previews inlined in react-native-live-markdown.

jeremy-croff commented 10 months ago

Personally I think this issue kind of "beats around the bush" – I would love to see our efforts spent towards building full support for inline images in messages, GitHub-style, with image previews inlined in react-native-live-markdown.

Is this really how our customers would use it? Slack for example does not have inline images in messages, cause it's just chat. This allows the full image to be previewed that's attached without it being cumbersome for the content your typing.

Jira and GitHub have inline messages, where the image is converted with the upload path because it's used for steps and detailed information, it's like a whole mini text editor. I think those use cases are more prosumer, rather than consumer.

roryabraham commented 10 months ago

My opinion is that the inline image support offered by GitHub and Jira is superior in every way to the way images are handled by Slack, WhatsApp, iMessage, etc...

Those platforms don't make it any easier to add images to a message. The only advantage over GitHub for example is that you don't have to switch to the Preview tab to see how the image looks in your final message. But that limitation won't apply in New Expensify.

I believe that the inline image experience I'm proposing would be better than any of the other platforms we've listed here and as such would be a differentiating factor in our chat platform.

quinthar commented 9 months ago

I agree with @roryabraham -- this is just a step in that direction. A more complete plan is here: https://expensify.slack.com/archives/C01GTK53T8Q/p1708849229702429?thread_ts=1707270017.261459&cid=C01GTK53T8Q

quinthar commented 9 months ago

I think we should start with this to fix our current image support, but I agree we should add true inline image support -- and ultimately, large object support as well. But that's a bigger project that probably warrants a design doc, this is more low hanging fruit.

tgolen commented 9 months ago

I confirmed a few things via Slack, and it sounds like this is the detailed path to move forward on this issue:

quinthar commented 8 months ago

https://github.com/Expensify/App/issues/37246 is on staging; I'm taking this off hold! @tgolen can you give an ETA for when you can take it up?

tgolen commented 8 months ago

I'll bump this up to daily and will add an ETA once I finish catching up on my other issues today.

tgolen commented 8 months ago

Daily Update

Next Steps

ETA

tgolen commented 8 months ago

I ran into a small snag with this today once I realized that the backend changes will require corresponding frontend changes to the optimistic data. I posted in slack about this to discuss a possible solution.

tgolen commented 8 months ago

Daily Update

Next Steps

ETA

tgolen commented 8 months ago

The slack discussion was resolved and this will be the rollout plan to ensure nothing breaks:

Background context: There are two API commands on the backend:

Problem: The backend changes to combine text and attachments into a single message also require corresponding frontend changes to support the proper creation of optimistic data. Without both frontend and backend changes being deployed at the same time, attachments will be broken until both changes are deployed (which could be several days or weeks).

Solution: Create a third API command called AddAttachmentAndText which will implement the new combined text+attachment message, and leave the other two API commands alone. Once the new API command has been deployed on the server, then the frontend can be updated to handle the new optimistic data format and switch from calling AddAttachment to AddAttachmentAndText.

tgolen commented 8 months ago

Daily Update

Next Steps

ETA

tgolen commented 8 months ago

Daily Update

Next Steps

ETA

tgolen commented 8 months ago

Daily Update

Next Steps

ETA

tgolen commented 8 months ago

Daily Update

Next Steps

ETA

tgolen commented 8 months ago

Daily Update

Next Steps

ETA

tgolen commented 8 months ago

Daily Update

Next Steps

ETA

[!NOTE] I'm going to be OOO tomorrow and Monday so the ETA might get off a little if there are actions required while I am gone

rayane-djouah commented 8 months ago

Reviewing the PR as C+, please assign me to the issue and add Bug and External labels for C+ payment. Thank you!

tgolen commented 7 months ago

@rayane-djouah It won't let me assign this to you until you comment on it. Can you please add a comment here and then I'll assign it.

tgolen commented 7 months ago

Wait... you just commented above me :D weird

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @bfitzexpensify (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.

tgolen commented 7 months ago

@bfitzexpensify can you please ensure @rayane-djouah gets paid for a C+ review of the PR?

bfitzexpensify commented 7 months ago

PR was assigned for review (and the review began) before the price for a PR review was updated, so payment for @rayane-djouah is $500. Offer sent via Upwork.

Looks like the PR is merged but not yet deployed. I've subscribed and will update the payment date here.

melvin-bot[bot] commented 7 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot[bot] commented 7 months ago

@tgolen, @bfitzexpensify, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

bfitzexpensify commented 7 months ago

Deployed today, updated title.

melvin-bot[bot] commented 7 months ago

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

quinthar commented 7 months ago

Should this be tagged Awaiting Payment? Why wasn't it automatically?

melvin-bot[bot] commented 7 months ago

@tgolen, @bfitzexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!

bfitzexpensify commented 7 months ago

All done here.