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 document picking support to mobile #689

Merged
merged 24 commits into from
Oct 28, 2020
Merged

Conversation

Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Oct 21, 2020

CC @Jag96

Add ability to add document attachments to chats on mobile. This already exists for web and desktop, but mobile users were only able to upload images.

  • Uses DocumentPicker library for native file selection
  • Renames our existing ImagePicker class to AttachmentPicker
  • Builds custom 'Add document' option into existing lists

Fixed Issues

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

Tests

1) Take Photo

  • Press attachment button, then 'Take photo'
  • The temporary attachment spinner should display
  • Photo should be uploaded as an attachment

2) Upload photo from gallery

  • Press attachment button, then 'Choose from gallery'
  • The temporary attachment spinner should display
  • Photo should be uploaded as an attachment

3)

  • Press attachment button, then 'Choose Document'
  • Select a file for upload (.pdf, .csv, ect)
  • File should be uploaded as an attachment

4)

  • Press attachment button, press cancel button
  • The temporary attachment spinner should not display

5)

  • Press attachment button, press 'Choose Document' button
  • Press cancel on the modal that pops up
  • The temporary attachment spinner should not display

6)

  • Revoke camera permission (or completely reinstall)
  • Press attachment button, press 'Take Photo' button
  • Deny permission
  • The temporary attachment spinner should not display

Screenshots

Simulator Screen Shot - iPhone 11 - 2020-10-23 at 11 23 02
Simulator Screen Shot - iPhone 11 - 2020-10-23 at 12 11 11

Android
Screenshot_1603450582
Screenshot_1603451295

@Julesssss Julesssss self-assigned this Oct 21, 2020
@Julesssss
Copy link
Contributor Author

Julesssss commented Oct 21, 2020

CC @Jag96. My thinking is that the previous ImagePicker should be instead called AttachmentPicker -- as it's responsible for more than just images. After that, it made sense to move both the Image and Document picking logic into the AttachmentPicker. Would be great to get your thoughts at this stage and to confirm the overall structure makes sense.

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

I like the idea of moving the Document picker inside the newly renamed AttachmentPicker. Left a couple of comments but looks good so far.

src/libs/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved
src/libs/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved
@Julesssss Julesssss force-pushed the jules-addMobileDocumentPicker branch from 2df85b5 to 8c2b9bd Compare October 22, 2020 16:42
@Julesssss Julesssss force-pushed the jules-addMobileDocumentPicker branch from 8c2b9bd to 686029c Compare October 22, 2020 16:49
@Julesssss
Copy link
Contributor Author

Julesssss commented Oct 23, 2020

Document upload is working, but I'm unsure about a certain behaviour:

When uploading a csv file from web, it is a downloadable file, but when I re attach it from mobile it is parsed and displayed as an image. I imagine this was desired for parsing expense reports, but should chat behave differently?

Simulator Screen Shot - iPhone 11 - 2020-10-23 at 12 17 35

@Julesssss Julesssss changed the title [WIP] Add document picking support to mobile Add document picking support to mobile Oct 23, 2020
@Julesssss Julesssss requested a review from a team October 23, 2020 11:30
@botify botify requested review from stitesExpensify and removed request for a team October 23, 2020 11:30
@Julesssss Julesssss marked this pull request as ready for review October 23, 2020 11:32
@shawnborton
Copy link
Contributor

I would think it should upload as a file. It would be awesome if we had file previews at some point, but that's a different feature.

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Looking good, just a couple of questions/recommendations

src/libs/AttachmentPicker/index.js Outdated Show resolved Hide resolved
src/libs/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved
src/libs/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved
src/libs/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved
Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

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

LGTM

Jag96
Jag96 previously approved these changes Oct 26, 2020
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

LGTM, just a nab

src/libs/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved
@Julesssss Julesssss dismissed stale reviews from Jag96 and stitesExpensify via 71eae67 October 28, 2020 16:25
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

LGTM, good stuff!

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.

4 participants