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

Add Modal Previews when Uploading Attachments + Copy/Paste files #685

Merged
merged 25 commits into from
Dec 9, 2020

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Oct 21, 2020

cc @thienlnam since most of the original code is yours just reorganized a little

Summary of Changes

  • Refactored attachment preview logic so the modal can be used both before and after a file is uploaded
  • Separated the "thumbnail" image from the ThumbnailWithModal component so that we can launch the preview modal without requiring a thumbnail to be displayed at all
  • Exposed modal launching methods via render prop so we can open image previews regardless of UI trigger
  • Added the ability to paste a file into the text input and launch the preview modal

TODO

  • Handle previews for non image attachments

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/143636

Tests (All platforms)

  1. Upload an attachment via the paperclip icon
  2. Verify you see a preview modal with an "upload" button
  3. Tap the button
  4. Verify it starts uploading and succeeds

Web / Desktop only tests

  1. Copy and paste an image file into the text input for a chat (does not work with non-image files)
  2. Verify you see a preview modal with an "upload" button
  3. Tap the button
  4. Verify it starts uploading and succeeds

Screenshots

iOS
Screen Shot 2020-12-01 at 3 05 11 PM

Android
Screen Shot 2020-12-01 at 4 25 24 PM

Web
Screen Shot 2020-12-01 at 3 09 00 PM

@marcaaron marcaaron self-assigned this Oct 21, 2020
@marcaaron marcaaron requested a review from Jag96 October 21, 2020 02:24
@marcaaron marcaaron changed the title Allow pasting files on web and desktop [HOLD] Allow pasting files on web and desktop Oct 21, 2020
@marcaaron
Copy link
Contributor Author

Throwing a hold on this as I think maybe we want to make sure that previewing attachments works before implementing this.

@marcaaron marcaaron requested a review from a team as a code owner December 1, 2020 22:18
@botify botify requested review from alex-mechler and removed request for a team December 1, 2020 22:18
@marcaaron marcaaron marked this pull request as draft December 1, 2020 22:19
@marcaaron marcaaron changed the title [HOLD] Allow pasting files on web and desktop Add Modal Previews for Attachments / Allow Copy/Paste of files Dec 1, 2020
@marcaaron marcaaron changed the title Add Modal Previews for Attachments / Allow Copy/Paste of files Add Modal Previews when Uploading Attachments / Allow Copy/Paste of files Dec 1, 2020
@marcaaron marcaaron changed the title Add Modal Previews when Uploading Attachments / Allow Copy/Paste of files Add Modal Previews when Uploading Attachments + Copy/Paste files Dec 1, 2020
@marcaaron marcaaron requested review from thienlnam and removed request for alex-mechler December 1, 2020 23:22
@marcaaron marcaaron changed the title Add Modal Previews when Uploading Attachments + Copy/Paste files Add Modal Previews when Uploading Attachments Dec 2, 2020
@marcaaron marcaaron changed the title Add Modal Previews when Uploading Attachments Add Modal Previews when Uploading Attachments + Copy/Paste files Dec 2, 2020
@marcaaron
Copy link
Contributor Author

Still working out how to handle non image attachments, but made some progress here.

@marcaaron
Copy link
Contributor Author

Got something like this so far if you want to poke around @shawnborton

Screen Shot 2020-12-02 at 3 14 16 PM

@shawnborton
Copy link
Contributor

Nice, that should be plenty to get started.

@marcaaron marcaaron requested review from roryabraham and removed request for Jag96 and AndrewGable December 7, 2020 22:47
@marcaaron
Copy link
Contributor Author

Gonna take this off hold so we can start getting some thoughts. There are a lot of changes but mostly it's copy and paste from files being renamed / reorganized. Would like to get this merged sooner than later as the conflicts are particularly painful to handle.

@shawnborton are you cool with doing a styling follow up for this so we can get a first draft merged and then focus on the non image/pdf attachment previews?

@marcaaron marcaaron marked this pull request as ready for review December 7, 2020 22:51
};

const AttachmentView = props => (
<>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, do you know why this was wrapped with a React.Fragment before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think was enabling us to use the short circuit syntax as a curly brace after the => would have opened a new block and required an explicit return.

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this is much better than it was before. There were a few pieces that were pretty hard to wrap my head around, but each time I tried to find a way to simplify things or make them clearer I ended up realizing why you did it the way you did and couldn't come up with a cleaner way to do it. It seems to work pretty well, but I'm not approving just yet because I need to do more testing.

src/components/ImageView/index.native.js Outdated Show resolved Hide resolved
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this looks good to me and seems to work well.

Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This tests well, and the images are super snappy
Screen Shot 2020-12-09 at 11 38 29 AM

@mananjadhav
Copy link
Collaborator

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issues in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants