-
Notifications
You must be signed in to change notification settings - Fork 94
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(editor): add "Open in new tab" option for link previews #6674
base: main
Are you sure you want to change the base?
feat(editor): add "Open in new tab" option for link previews #6674
Conversation
Dear @pbirrer Thanks a lot for your contribution. I really like the idea and have been missing this in the past. However this could also be a follow up. I will add this PR to my review list and do a code review as soon as possible. |
From a usability point of view I would also prefer a direct button; if this is okay with the NC team. It would make the function much more obvious. |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
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.
Code looks good. There are a few test failures because of the additional href
property in extractLinkParagraph
. They are fairly easy to spot in the Files changed view here on github.
@pbirrer could you try and fix the tests?
Please let me know if you need any help with this. |
This comment was marked as resolved.
This comment was marked as resolved.
I would say a button is still great to have in addition to the clickable link preview. Especially since the link preview might be interactive or rendered by external apps, so I'd always prefer to still have a common way of just opening the link |
c12a909
to
8a87b02
Compare
Thanks, I also agree that a button to open the link is still a good idea. But I wonder whether the button should be in the link bubble instead of the three-dot-menu. I perceive the three dot menu more like a "config menu", also something normal users probably won't spot that easily. Most people will just click on the link and see the link bubble that opens. I would expect a "Open in new tab" button there. |
@mejo-: From what I can tell, the link bubble becomes inaccessible in preview mode. To resolve this, the button could be placed nearby—perhaps beside or below the triple-dot button? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6674 +/- ##
==========================================
+ Coverage 46.76% 46.79% +0.03%
==========================================
Files 748 733 -15
Lines 34163 34156 -7
Branches 1242 1227 -15
==========================================
+ Hits 15976 15985 +9
+ Misses 17565 17564 -1
+ Partials 622 607 -15 ☔ View full report in Codecov by Sentry. |
Just to clarify - that would mean the same action in two places, right? |
All that said... I will fix the linter and prepare this for merging as it's already an improvement over the current state. We can move the link to more meaningful places (i.e. into the link bubble and the preview) as a follow up. |
8a87b02
to
f0e350f
Compare
- Enhanced `PreviewOptions.vue` to include a new "Open in new tab" button for link previews. - Introduced the `openLink` method to open the link in a new browser tab. - Added `OpenIcon` for the button to improve UI consistency. - New `href` prop added to handle link URLs dynamically. - Updated `extractLinkParagraphs.js` to extract `href` attributes from nodes. - Ensures link previews include the necessary `href` for the "Open in new tab" action. These changes improve the user experience by allowing quick access to link previews in the editor, streamlining navigation workflows. Signed-off-by: Peter Birrer <[email protected]> Signed-off-by: Max <[email protected]>
f0e350f
to
2c673b6
Compare
Thanks to everyone who looks into this. I just want to stress the fact that non-techy users expect a link to be accessible through clicking the link. So I still think that the link bubble that opens when clicking the link (the text one, not the preview) should have a "open in new tab" button in my opinion. That's also been requested here: nextcloud/collectives#1226 For link previews (where the link bubble doesn't show) I would prefer to have the button as an overly button next to the "edit" button, like @max-nextcloud suggested. When I watch people using Text/Collectives, my impression is that the three-dots menu left to links often remains unnoticed. Plus, as @juliusknorr already pointed out, it's only displayed for links that span over a separate paragraph, not for inline links. |
This update aims to improve accessibility to files added as previews in the text editor.
📝 Summary
Enhanced
PreviewOptions.vue
to include a new "Open in new tab" button for link previews.openLink
method to open the link in a new browser tab.OpenIcon
for the button to improve UI consistency.href
prop added to handle link URLs dynamically.Updated
extractLinkParagraphs.js
to extracthref
attributes from nodes.href
for the "Open in new tab" action.🖼️ Screenshots
🏁 Checklist
npm run lint
/npm run stylelint
/composer run cs:check
)