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

#2794 fix document attachment label #2919

Merged

Conversation

dklymenk
Copy link
Contributor

Details

Documentation for RNDocumentPicker says that the returned object has name and not fileName key.

Fixed Issues

Fixes #2794

Tests/QA Steps

PDF attachment:

  1. Login in chat app.
  2. Go to a conversation
  3. Tap on the attachment icon
  4. Tap on document
  5. Select a PDF document.
  6. You should see a preview

non-PDF non-image attachment:

  1. Login in chat app.
  2. Go to a conversation
  3. Tap on the attachment icon
  4. Tap on document
  5. Select a non-PDF non-image file (I used .xsl).
  6. You should see a preview with file name only.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

PDF

Simulator Screen Shot - iPhone 11 - 2021-05-14 at 03 06 37

non-PDF non-image

Simulator Screen Shot - iPhone 11 - 2021-05-14 at 03 07 28

Android

PDF

Screenshot_1620951012

non-PDF non-image

Screenshot_1620951078

@dklymenk dklymenk requested a review from a team as a code owner May 14, 2021 00:19
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team May 14, 2021 00:19
@dklymenk
Copy link
Contributor Author

dklymenk commented May 14, 2021

The diff affects only one .native.js file, so to my understanding only iOS and Android should be tested.

Please let me know if I should add screenshots from other clients.

@chiragsalian
Copy link
Contributor

Bump @thienlnam to review.

@thienlnam
Copy link
Contributor

Tested IOS last friday (Worked well), currently running into issues with my android emulator so I will merge once I get that sorted out and tested

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, works as expected!

@thienlnam thienlnam merged commit 186402d into Expensify:main May 17, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@dklymenk dklymenk deleted the 2794-fix-document-attachment-label branch May 18, 2021 12:32
name: fileData.fileName || 'chat_attachment',
name: fileData.name || 'chat_attachment',
Copy link
Contributor

Choose a reason for hiding this comment

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

This fixes the issue for the document picker but breaks it for the image picker as both use this function
one would call it with fileData that has fileName - image-picker results, the other with fileData that has just name
This should have been changed to

name: fileData.fileName || fileData.name || 'chat_attachment'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I agree.
@thienlnam , do you want me to create a new MR for this particular change or should this be a part of MR for this new issue @kidroca has created?

Copy link
Contributor

@thienlnam thienlnam Jun 7, 2021

Choose a reason for hiding this comment

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

I'd recommend creating a new pull request with those changes, tag the new issue that @kidroca created and then add me as a reviewer on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thienlnam
Here is the PR. I can't manually add you to the list of reviewers.
#3408

Thanks

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.

Attaching document in chat shows a default name of "Chat_attachement" instead of the document name
5 participants