-
Notifications
You must be signed in to change notification settings - Fork 289
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
feat: Use anchor tag for link preview #15039
Conversation
Gentle ping for this MR. The current behavior can be frustrating and the fix is very simple |
@atomrc : Sorry if you are not the right person to contact, I didn't find the information on the README. Almost every day, I have to look at the DOM of the application to copy links/open in private windows because the links are not in anchor tags. Do you think it is possible to review this PR ? |
src/script/components/MessagesList/Message/ContentMessage/asset/LinkPreviewAssetComponent.tsx
Outdated
Show resolved
Hide resolved
className="link-preview-info-link text-foreground ellipsis" | ||
href={preview.url} |
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.
Please use the prependProtocol
function on the preview.url
from UrlUtil
to make sure the link has the right protocol.
href={prependProtocol(preview.url)}
Hi @p1gp1g and thanks for your contribution 👌 A few things to consider that I have added as a review. Thanks again and let me know if you have any question :) |
ef5e316
to
580dc8b
Compare
Thanks for your review ! I have amended the commit with your changes Note: noreferrer alone should be enough: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel#noreferrer |
This allows one to use the browser's context menu for instance to copy the link or open it in a new window.
580dc8b
to
59d0a4b
Compare
Codecov Report
@@ Coverage Diff @@
## dev #15039 +/- ##
==========================================
+ Coverage 43.42% 44.48% +1.06%
==========================================
Files 645 672 +27
Lines 21582 22707 +1125
Branches 4945 5167 +222
==========================================
+ Hits 9372 10102 +730
- Misses 11025 11316 +291
- Partials 1185 1289 +104 |
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.
Approved 😎
Thanks @p1gp1g
This allows one to use the browser's context
menu for instance to copy the link or open
it in a new window.
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
We currently can't use the browser's context menu when a link is previewed. We can't copy/open in new (private) windows etc.
Causes (Optional)
The link previews use a paragraph tag.
Solutions
The link previews use an anchor tag.
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.