-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Link Previews #17288
Link Previews #17288
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
@AndrewGable 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] |
Wow this is awesome! 😍 |
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.
Looks nice! Adding some more details about the next approach.
cc @thienlnam |
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.
Looks great and thanks for the work on this! I had a few small style notes and a comment about the limiting of previews (not sure if we need to do it yet).
@wojtus7 Please let me know if I can explain anything else or help move things along 🙇 |
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.
Looking good, I will ask for some C+ for testing but I have noticed we should multiple of the link previews which is not intended, is it?
I see your comment I misread that was for same links. Where do we add the limit for 3 link previews in the code?
} | ||
|
||
Report.expandURLPreview(props.report.reportID, props.action.reportActionID); | ||
}, []) |
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.
Linter warning here
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 url expands only on component mount. Is it already planned to refresh preview on edit?
Screen.Recording.2023-05-25.at.3.31.31.PM.mov
Do they also work when you insert a markdown link? |
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.
@wojtus7 author checklist is outdated. please pull from latest main.
Please check and fix this everywhere.
- [x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. `myBool && <MyComponent />`.
} | ||
|
||
Report.expandURLPreview(props.report.reportID, props.action.reportActionID); | ||
}, []) |
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 url expands only on component mount. Is it already planned to refresh preview on edit?
Screen.Recording.2023-05-25.at.3.31.31.PM.mov
@rushatgabhane yes works Screen.Recording.2023-05-25.at.3.43.15.PM.mov |
really cool feature btw! |
@wojtus7 have you posted the CLA comment? |
@wojtus7 I also noticed all commits are not signed. |
I have read the CLA Document and I hereby sign the CLA |
a9e35cf
to
940cf8b
Compare
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.
couple of NABs
@0xmiroslav can you please give this one another go, seems like we are close
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
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
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
I don't think it's related to this PR. That was happening some days ago (before this PR merged) but now I am not able to reproduce. |
I am not sure the changes here dont really touch that. Might be still worth trying revert locally |
bug.movOn latest main, I noticed new link message is auto-flagged every time. |
Sorry folks for the scare, looks like this was fixed in the backend and I had checked out a different branch before and completely forgot. |
@wojtus7 nvm, please ignore #17288 (comment). We confirmed it's backend issue and already happens on production. |
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.28-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
const regex = /<a\s+(?:[^>]*?\s+)?href="([^"]*)"/gi; | ||
|
||
if (!htmlContent) { | ||
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.
BTW this caused a bug which I fixed here https://github.com/Expensify/App/pull/21369/files
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 for catching it 🙇
@@ -328,6 +340,11 @@ function ReportActionItem(props) { | |||
return ( | |||
<> | |||
{children} | |||
{!_.isEmpty(props.action.linkMetadata) && ( | |||
<View style={props.draftMessage ? styles.chatItemReactionsDraftRight : {}}> | |||
<LinkPreviewer linkMetadata={_.filter(props.action.linkMetadata, (item) => !_.isEmpty(item))} /> |
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 has caused a regression in #21195
We shouldn't show link preview if message was flagged as inappropriate, this was resolved by hiding LinkPreview
if isHidden=true
👋 Coming from #22888, |
Details
Added link preview in messages that contain links.
Up to three link previews are visible for single message but it can be changed using one variable.
If multiple the same links are provided only one preview will be displayed.
Fixed Issues
Part of #15001
Added based on:
https://docs.google.com/document/d/1317yQGmDgDnjwfoLybqNb5qVUU_jkhI2Bs3X9PQFcG4/edit#heading=h.fcvmbish8sz6
Tests
Offline steps
If cached earlier downloaded previews will show as usual, if there is no cache nothing will download so nothing should show up if there is no network connection.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Screen.Recording.2023-04-11.at.13.42.20.mov
Android