Change UI view for Attachments in Chat #4736

akshayasalvi · 2021-08-18T17:43:42Z

Details

Fixed Issues

$ #4601

Tests

QA Steps

Go to any chat
Upload any word document (docx) or non-preview document
Verify the new attachment block UI
Also clicking on download should give the same filename as preview

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions · 2021-08-18T17:44:00Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

thienlnam · 2021-08-18T17:53:03Z

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

@@ -67,6 +67,8 @@ function AnchorRenderer({tnode, key, style}) {

    // An auth token is needed to download Expensify chat attachments
    const isAttachment = Boolean(htmlAttribs['data-expensify-source']);
+    const fileName = isAttachment && tnode.domNode && tnode.domNode.children && tnode.domNode.children[0] && tnode.domNode.children[0].data ? tnode.domNode.children[0].data : null;


Let's use lodashGet to clean this up a bit

Okay will do.

akshayasalvi · 2021-08-18T18:11:42Z

I have read the CLA Document and I hereby sign the CLA

thienlnam · 2021-08-18T20:52:35Z

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.js

-                href={href}
-                hrefAttrs={{rel, target}}
+
+        props.shouldDownloadFile


Let's update this prop to isAttachment

thienlnam · 2021-08-18T20:59:21Z

src/components/AttachmentView.js

+            {props.rightElement && (
+                <View style={styles.ml2}>
+                    {props.rightElement}
+                </View>
+            )}


Instead of passing down rightElement as props, pass down a prop called shouldShowDownloadIcon to conditionally render the download icon

thienlnam · 2021-08-18T20:59:45Z

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.js

-                href={href}
-                hrefAttrs={{rel, target}}
+
+        props.shouldDownloadFile


update prop name to isAttachment

thienlnam · 2021-08-18T21:00:36Z

src/components/AnchorForCommentsOnly/anchorForCommentsOnlyPropTypes.js

+
+    text: PropTypes.string,


Move this prop above style, and add comment
Also rename from text to fileName

thienlnam · 2021-08-18T21:01:04Z

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

@@ -81,6 +84,7 @@ function AnchorRenderer({tnode, key, style}) {
            rel={htmlAttribs.rel || 'noopener noreferrer'}
            style={style}
            key={key}
+            text={fileName}


like above, change text prop name to fileName

thienlnam · 2021-08-18T21:06:35Z

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

@@ -67,6 +68,8 @@ function AnchorRenderer({tnode, key, style}) {

    // An auth token is needed to download Expensify chat attachments
    const isAttachment = Boolean(htmlAttribs['data-expensify-source']);
+    const fileName = isAttachment ? lodashGet(tnode, 'domNode.children[0].data') : null;


Add a default param to lodashGet(tnode, 'domNode.children[0].data') of an empty string '' and remove the conditional and just use that

akshayasalvi · 2021-08-18T22:51:12Z

@thienlnam PR updated. Let me know if any other changes.

…nt-view-in-comments

thienlnam · 2021-08-20T01:22:57Z

src/components/AnchorForCommentsOnly/anchorForCommentsOnlyPropTypes.js

+    /** Flag to differentiate attachments and hyperlink. Base on flag link will be treated as a file download or a regular hyperlink?
+     * (relevant to native platforms only)  */


Remove the ? and (relevant to native platforms only) since it applies to all platforms

thienlnam · 2021-08-20T01:24:29Z

Changes look good, just the small name comment. I tested and it works as expected, but the file download name isn't the same name that is shown, it is still the name of a randomly generated string

akshayasalvi · 2021-08-20T02:56:39Z

@thienlnam I have done those changes too but it doesn't work. I am getting a CORS issue when the fileDownload code works and then it ends up opening the URL in the catch block Linking.openURL(url);. If this can be fixed then I can push those changes too.

thienlnam

I've been doing some testing and am not getting the downloaded file to match the file name - are you experiencing the same issue?

akshayasalvi · 2021-08-23T17:10:57Z

Are you checking from localhost? That's what I was mentioning in the previous comment, I get a CORS error. Hence, fetch(url) fails, and the following block is executed.

}).catch(() => {
     // file could not be downloaded, open sourceURL in new tab
     Linking.openURL(url);
});

thienlnam · 2021-08-23T17:34:13Z

@akshayasalvi Gotcha, yeah that's where I've been testing - going to merge and we'll see if expected behavior still happens on prod

akshayasalvi · 2021-08-23T18:08:01Z

@thienlnam Thanks for approving. We should be able to test this on staging right?

thienlnam · 2021-08-23T18:18:59Z

@akshayasalvi Yup, will be tested on staging before it goes live on production (sorry, should have clarified)

OSBotify · 2021-08-23T18:19:25Z

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

isagoico · 2021-08-26T02:13:56Z

@akshayasalvi Hello! Any QA tests needed for this PR?

akshayasalvi · 2021-08-26T04:32:54Z

@isagoico I've added the QA steps

OSBotify · 2021-08-30T17:38:06Z

🚀 Deployed to production by @roryabraham in version: 1.0.88-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

botify · 2021-09-01T01:26:09Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-08. 🎊

akshayasalvi added 4 commits August 18, 2021 23:08

Added rightElement for AttachmentView

2e2b39f

Fetch Filename from the textNode (to be changed)

7c57fc9

replaced hyperlink with attachmentview based on condition

9b6706c

pass filename as prop

7b52c25

thienlnam reviewed Aug 18, 2021

View reviewed changes

cleaned up the data fetch with lodash.get

622fef0

Added attachment layout to native devices

924271f

akshayasalvi marked this pull request as ready for review August 18, 2021 18:55

akshayasalvi requested a review from a team as a code owner August 18, 2021 18:55

MelvinBot requested review from thienlnam and removed request for a team August 18, 2021 18:55

thienlnam requested changes Aug 18, 2021

View reviewed changes

akshayasalvi added 3 commits August 19, 2021 04:02

changed prop name to fileName

f658da2

Renamed prop to isAttachment

cab465c

Moved download icon to AttachmentView with a flag for hide/show

5518ed2

Merge branch 'main' of github-personal:akshayasalvi/App into attachme…

ef75235

…nt-view-in-comments

thienlnam requested changes Aug 20, 2021

View reviewed changes

akshayasalvi added 2 commits August 20, 2021 18:53

Added fileName param to filedownload logic

17b84d6

Removed console.error from catch block

dae136d

akshayasalvi requested a review from thienlnam August 20, 2021 21:17

updated comment as per suggestion

1929de1

thienlnam requested changes Aug 23, 2021

View reviewed changes

thienlnam approved these changes Aug 23, 2021

View reviewed changes

thienlnam merged commit 90bff75 into Expensify:main Aug 23, 2021

This was referenced Aug 24, 2021

Deploy Checklist: New Expensify 2021-08-17 #4701

Closed

Deploy Checklist: New Expensify 2021-08-25 #4828

Closed

isagoico mentioned this pull request Aug 27, 2021

Download file doesn't have the same name as preview #4868

Closed

kavimuru mentioned this pull request Aug 27, 2021

Chat - Clicking on download does not show same filename as in the preview #4869

Closed

parasharrajat mentioned this pull request Aug 27, 2021

Chat - No tool tip for download icon for docx file #4870

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change UI view for Attachments in Chat #4736

Change UI view for Attachments in Chat #4736

akshayasalvi commented Aug 18, 2021 •

edited

Loading

github-actions bot commented Aug 18, 2021 •

edited

Loading

thienlnam Aug 18, 2021

akshayasalvi Aug 18, 2021

akshayasalvi Aug 18, 2021

akshayasalvi commented Aug 18, 2021

thienlnam Aug 18, 2021

akshayasalvi Aug 18, 2021

thienlnam Aug 18, 2021

akshayasalvi Aug 18, 2021

thienlnam Aug 18, 2021

akshayasalvi Aug 18, 2021

thienlnam Aug 18, 2021

akshayasalvi Aug 18, 2021

thienlnam Aug 18, 2021

akshayasalvi Aug 18, 2021

thienlnam Aug 18, 2021

akshayasalvi Aug 18, 2021

akshayasalvi commented Aug 18, 2021

thienlnam Aug 20, 2021

akshayasalvi Aug 20, 2021

thienlnam commented Aug 20, 2021

akshayasalvi commented Aug 20, 2021

thienlnam left a comment

akshayasalvi commented Aug 23, 2021

thienlnam commented Aug 23, 2021

akshayasalvi commented Aug 23, 2021

thienlnam commented Aug 23, 2021

OSBotify commented Aug 23, 2021

isagoico commented Aug 26, 2021

akshayasalvi commented Aug 26, 2021

OSBotify commented Aug 30, 2021

botify commented Sep 1, 2021

		/** Flag to differentiate attachments and hyperlink. Base on flag link will be treated as a file download or a regular hyperlink?
		* (relevant to native platforms only) */

Change UI view for Attachments in Chat #4736

Change UI view for Attachments in Chat #4736

Conversation

akshayasalvi commented Aug 18, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayasalvi commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayasalvi commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thienlnam commented Aug 20, 2021

akshayasalvi commented Aug 20, 2021

thienlnam left a comment

Choose a reason for hiding this comment

akshayasalvi commented Aug 23, 2021

thienlnam commented Aug 23, 2021

akshayasalvi commented Aug 23, 2021

thienlnam commented Aug 23, 2021

OSBotify commented Aug 23, 2021

isagoico commented Aug 26, 2021

akshayasalvi commented Aug 26, 2021

OSBotify commented Aug 30, 2021

botify commented Sep 1, 2021

akshayasalvi commented Aug 18, 2021 •

edited

Loading

github-actions bot commented Aug 18, 2021 •

edited

Loading