-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Added the ability to paste Google Docs images / charts into the Composer chat for web based platforms #34971
Conversation
@Santhosh-Sellavel Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/components/Composer/index.tsx
Outdated
if (clipboardDataHtml?.includes(CONST.IMAGE_BASE64_MATCH)) { | ||
const domparser = new DOMParser(); | ||
const pastedHTML = clipboardDataHtml; | ||
const embeddedImages = domparser.parseFromString(pastedHTML, TEXT_HTML)?.images; | ||
|
||
if (embeddedImages.length > 0 && embeddedImages[0].src) { | ||
const src = embeddedImages[0].src; | ||
|
||
// convert base64 to file blob | ||
const base64ImageContent = src.split(',')[1]; | ||
const mimeType = src.split(';')[0].split(':')[1]; | ||
const byteString = atob(base64ImageContent); | ||
const arrayBuffer = new ArrayBuffer(byteString.length); | ||
const uint8Array = new Uint8Array(arrayBuffer); | ||
for (let i = 0; i < byteString.length; i++) { | ||
uint8Array[i] = byteString.charCodeAt(i); | ||
} | ||
const blob = new Blob([arrayBuffer], {type: mimeType}); | ||
const file = new File([blob], 'image.jpg', {type: 'image/jpeg'}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make use of this available method base64ToFile
, DRY up the logic here.
App/src/libs/fileDownload/FileUtils.ts
Line 213 in 82fff8c
function base64ToFile(base64: string, filename: string): File { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, I missed that one. Thanks!
src/components/Composer/index.tsx
Outdated
// If the clipboardDataHtml contains google sheets html data (pasted chart) | ||
if (clipboardDataHtml?.includes(CONST.GOOGLE_SHEETS_HTML_MATCH)) { | ||
const domparser = new DOMParser(); | ||
const pastedHTML = clipboardDataHtml; | ||
const dom = domparser.parseFromString(pastedHTML, TEXT_HTML); | ||
const sheetHTML = dom.getElementsByTagName('google-sheets-html-origin')?.item(0); | ||
const table = sheetHTML?.getElementsByTagName('table')?.item(0); | ||
|
||
if (sheetHTML) { | ||
document.body.appendChild(sheetHTML); | ||
|
||
// in case there's no table, we'll screenshot the whole pasted sheet | ||
html2canvas(table ?? (sheetHTML as HTMLElement)).then((canvas) => { | ||
const file = FileUtils.dataURItoBlob(canvas.toDataURL('image/png')); | ||
onPasteFile(file); | ||
}); | ||
|
||
// cleanup | ||
document.body.removeChild(sheetHTML); | ||
return; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not something we want to do here, i.e. we do not want to make any screenshots of objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, then I will remove that part.
I suppose we're not looking at having functionality that will allow to copy html objects like tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only copy actual objects like charts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, that's already being covered by the base64 block.
I'll clean it up right now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we're not looking at having functionality that will allow to copy html objects like tables.
We would have in the future, I guess. The table should be copied as a table in mark down, that's when it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor Comments, Looks good will continue testing & try and complete the checklist by tomorrow.
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeScreen.Recording.2024-01-26.at.1.56.22.AM.moviOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.-.2024-01-26.at.01.39.09.mp4MacOS: Chrome / SafariScreen.Recording.2024-01-26.at.1.27.18.movMacOS: DesktopScreen.Recording.2024-01-26.at.1.29.02.AM.mov |
@ikevin127 In this PR we are adding support to copy charts from google sheets, and images from from google workspaces apps like slide, docs, etc. So please update the test steps accordingly a bit more clearly for QA. Also, give example doc links or sheet links with charts copy is supported. Update the PR title also meaning fully. |
This comment was marked as outdated.
This comment was marked as outdated.
All done! 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tests well!
@ikevin127 can you sync with main please? |
@mountiny Sure, done! |
@ikevin127 i think that did not go well 😅 |
@mountiny My bad, should be good to go now. Btw for future references, how would you personally perform such a sync in order for it to go well ? |
@ikevin127 great questions which I personally do not have experience with since I dont have to use a fork of the repo. I assume you need to sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@ikevin127 well seems like there is still an issue with the reassure tests but that is resolved on main, can you try again? |
…ser chat for web based platforms
@mountiny Correct, the reassure tests were failing for a while but as of latest main it looks like that was fixed. Thank you! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.33-0 🚀
|
The QA team is reporting that this didn't fix the issue - not going to block the deploy on this but I'm removing the |
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.33-5 🚀
|
@francoisl I think I found the issue: while this was working without any issues on local development and both me and the reviewer's tests passed, there are some things we did not account to on staging / production in order to allow our app to fetch certain external links like the And this is why only Google Sheets image and chart (both base64) paste works because there are no external links involved on paste. Currently on staging this is what works and what doesn't:
Error text:
Which by the error message we can figure that our Content Security Policy directive: "connect-src" needs to be updated by adding the I think this can be fixed by a member of the team and it doesn't require pushing any other changes from this PR's point of view. I'm awaiting confirmation from a team member though! 🙌 |
PR raised internally |
Both scenarios work as expected since the internal PR was merged - covering Google Sheets / Slides / Docs images and charts. ✅ More details in the comment I posted on the issue @ #34306 (comment) Given the current expectations listed in the issue / PR's description and the fact that this was deployed to production 2 days ago - I guess the hold for payment should be added to the issue @ #34306 as the automation failed here. If we want to extend the functionality in the near future, I gather a new issue / feature request can be opened. I'd be happy to take on that as well when the time comes! 😁 |
@ikevin127 I've reopened #34306 (comment) to make sure payment happens! |
@conorpendergrast I'm a bit confused as to the hold for payment date assigned to the issue being 2024-02-14 👀 As far as I remember the hold is 7 days from the point when the deploy to production was completed. As per this comment #34971 (comment), the fix was deployed to production 2 days ago (2024-01-31, 4:32 EET) which would make the hold for payment date 2024-02-07 at the latest. Is there any reason why it's an extra week for this one or did the hold period increase by 1 week for everybody sometime recently ? ✌️ |
For absolutely no reason beyond my Friday brain. Thank you for pointing that out, I have fixed that to have the 7-day window |
Details
Added the ability to copy objects (not screenshots of objects, just the object itself) from Google Workspace into an Expensify chat, specifically:
Note: The composer objects paste functionality works only on Web, iOS / Android: mWeb and Desktop. On native platforms we never had the object paste functionality, not even for images as we have had on the other platforms already.
Fixed Issues
For #34306
PROPOSAL: #34306 (comment)
Tests
Web / Desktop steps:
iOS / Android mWeb steps:
Prerequisite: The Google Sheets / Google Slides / Google Docs apps have to be installed on the device in order to be able to copy images / charts.
Offline tests
Web / Desktop steps:
iOS / Android mWeb steps:
Prerequisite: The Google Sheets / Google Slides / Google Docs apps have to be installed on the device in order to be able to copy images / charts.
QA Steps
Web / Desktop steps:
iOS / Android mWeb steps:
Prerequisite: The Google Sheets / Google Slides / Google Docs apps have to be installed on the device in order to be able to copy images / charts.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android-native.mov
Android: mWeb Chrome
android-mweb.mp4
iOS: Native
ios-native.mov
iOS: mWeb Safari
ios-mweb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov