Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Link bubble menu to preview and edit links #5158

Merged
merged 15 commits into from
Jan 30, 2024
Merged

Link bubble menu to preview and edit links #5158

merged 15 commits into from
Jan 30, 2024

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Dec 23, 2023 •
edited
Loading

📝 Summary

Opens a link bubble when clicking on a link and removes in-document link previews.

  • Opens link bubble when clicking on a link or navigating inside a link mark.
  • Displays a link preview in the bubble. Loads a fallback if no preview can be loaded.
  • Actions inside the bubble: copy link to clipboard, edit link URL, remove link mark.
  • Anchor links (href starting with #) don't open a link bubble but navigate to the anchor directly.
  • No longer display link previews in the document.
  • Ctrl + click and middle click on a link doesn't open link bubble. Instead opens the link directly in target="_blank".
  • Esc closes the link bubble.
  • Remove the possibility to customize link click handlers. Instead, we use the reference widgets now, and they should provide their own click handlers if desired.
  • Resolves: When editing documents, clicking links will open a new Markdown editor collectives#845
  • Resolves: Middle click on links must not insert text #2198
  • Resolves: Links in Text files will open 2x #3691

🖼️ Screenshots

Overview
overview
Copy link to clipboard Edit link Remove mark Edit mode
clipboard edit1 remove edit
Screencast
recording1

🚧 TODO

  • Display link URL if no widget is loaded
  • Display link href instead of preview when !isLoggedIn
  • Read-only support
  • Remove link preview from paragraph node view
  • Refactor link handling callbacks
  • Manual testing
    • Check on mobile
    • Check in public shares
    • Check with references to collective page
  • Cypress tests (require fix: Fix tab focus when other elements are displayed next to text that are within a focus trap #5281 to pass)
  • Optional: Display link text in header of link bubble
  • Optional: Allow to set text to link title

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation is not required

@mejo- mejo- added enhancement New feature or request 2. developing labels Dec 23, 2023
@mejo- mejo- added this to the Nextcloud 29 milestone Dec 23, 2023
@juliushaertl
Copy link
Member

Looks very nice already. I stumbled over the following two issues that would be worth to check and address in the same run:

@mejo- mejo- force-pushed the feat/link_preview_click branch 6 times, most recently from 232ae47 to d1eda86 Compare January 16, 2024 05:48
@mejo- mejo- force-pushed the feat/link_preview_click branch 7 times, most recently from 8956f31 to 448d69b Compare January 22, 2024 16:58
max-nextcloud
max-nextcloud reviewed Jan 23, 2024
View reviewed changes
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall approach looks good to me. I added some minor comments but nothing blocking.

src/components/Link/LinkBubbleView.vue Outdated Show resolved Hide resolved
src/extensions/LinkBubblePluginView.js Outdated Show resolved Hide resolved
src/marks/Link.js Outdated Show resolved Hide resolved
@mejo- mejo- force-pushed the feat/link_preview_click branch 11 times, most recently from 5441d7a to 9d7dae3 Compare January 24, 2024 15:55
@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Jan 29, 2024 •
edited
Loading

Still much less of a drawback than not being able to append a word to the link name IMO.

I'm not sure why this is not working. I can append a word to an italic text fine - and from the tiptap perspective they are both marks.

update: looks like we inherit this behavior from both tiptap and prosemirror.

@juliushaertl
Copy link
Member

The discussion about adding a description field should not block this PR imo, as it would be just another additional new feature. Maybe we can move that to an issue to discuss as a potential follow up.

@juliushaertl
Copy link
Member

I'm not sure why this is not working. I can append a word to an italic text fine - and from the tiptap perspective they are both marks.
update: looks like we inherit this behavior from both tiptap and prosemirror.

Just for reference, quoting prosemirror docs:

By default, marks are inclusive, meaning that they get applied to content inserted at their end (as well as at their start when they start at the start of their parent node). For link-type marks, this is usually not the expected behavior, and the inclusive property on the mark spec can be set to false to disable that behavior.

And tiptap seems to auto set that if autolink is enabeld
https://github.com/ueberdosis/tiptap/blob/main/packages/extension-link/src/link.ts#L88

@nimishavijay
Copy link
Member

nimishavijay commented Jan 29, 2024 •
edited by mejo-
Loading

Agreed with both that the link text can be edited in the popup, and that it can be a follow up PR :) This PR looks really great! Only some minor details for feedback:

  • Indeed the image preview can be smaller, as at this size the text will likely be ellipsized most of the time. How about 64px? :)
  • There can also be more space between the image preview and the edges of the popup, so some margin can be added
  • The border radius of the image preview should be nicely aligned with border radius of the popup, currently it's too less
  • When you are editing the link, the right and left spacing seems pretty tight. I suggest some left and right margin be added to the text field
  • In the screenshots there is no text on the left of the buttons, but in the screencast it's there. Is that intentional or am I missing something? I suggest that there doesn't need to be text, but instead there can be an "Open link" secondary button (what do you think @marcoambrosini ?) :)

@mejo-
Copy link
Member Author

mejo- commented Jan 29, 2024

The failing Cypress test in workspaces.spec.js is because in CI we have http://localhost:8080 or something alike as local URL and NcReferenceList doesn't regard this as a valid URL. The following PR aims to change this: nextcloud-libraries/nextcloud-vue#5176

@max-nextcloud max-nextcloud mentioned this pull request Jan 30, 2024
Closed
@mejo-
Copy link
Member Author

mejo- commented Jan 30, 2024

Thanks for your feedback @marcoambrosini and @nimishavijay.

Indeed the image preview can be smaller, as at this size the text will likely be ellipsized most of the time. How about 64px? :)

Given that this would need to happen in the NcReferenceWidget component in nextcloud-vue, I'd suggest to make this a follow-up topic.

In the screenshots there is no text on the left of the buttons, but in the screencast it's there. Is that intentional or am I missing something? I suggest that there doesn't need to be text, but instead there can be an "Open link" secondary button (what do you think @marcoambrosini ?) :)

Addin an "open link" button is a bit more technically involved. The link preview from NcReferenceWidget implementations might have custom link handling implemented. Like opening files in the viewer, or opening other pages of a collective via re-routing in vue. Currently there is no easy way to "copy" this behaviour outside the widgets. So I'd rather move this to a follow-up as well.

@mejo-
feat(editor): Add link view bubble to preview, edit and open links
f750a82 
Signed-off-by: Jonas <[email protected]>
@mejo-
feat(paragraph): Don't show link previews
d14558b 
Signed-off-by: Jonas <[email protected]>
@mejo-
refactor(link): Improve readability of clickHandler function
da24829 
Signed-off-by: Jonas <[email protected]>
@mejo-
refactor(RichText): remove option to disable Link extension
df2e5c3 
This option is not used anyway. We always use the Link extension.

Signed-off-by: Jonas <[email protected]>
@mejo-
feat(editor): Refactor link click handlers
bbee8a0 
New link click behaviour:
* Link left clicks without Ctrl/Meta open link bubble (Fixes: #3691)
* Link left clicks with Ctrl/Meta open link in new tab
* Link middle clicks open link in new tab
* Link middle clicks on Linux don't paste content (Fixes: #2198)
* No more custom open link handler in editor

Implementation details:
* Moved link click handler plugins back into Link mark class.
* Added 'data-md-href' attribute to a-elements in DOM, to be used in
  onClick handlers.

Signed-off-by: Jonas <[email protected]>
@mejo-
fix(LinkBubble): Prevent update race condition in read-only in Chrome
dc13b01 
Signed-off-by: Jonas <[email protected]>
@mejo-
chore(LinkBubble): Remove code to customize link click handling
648cf20 
We no longer allow custom link click handlers in Text. Instead, the
reference widgets for link previews have to implement their own click
handlers.

Signed-off-by: Jonas <[email protected]>
@mejo-
test(cypress): Fix link tests with link bubble
9a81916 
Signed-off-by: Jonas <[email protected]>
@mejo-
fix(LinkBubble): Use private variables for LinkBubbblePluginView class
3702138 
Signed-off-by: Jonas <[email protected]>
@mejo-
fix(LinkBubble): Restore selection after updating the link
9f7da38 
Signed-off-by: Jonas <[email protected]>
@mejo-
fix(LinkBubble): Improved blur event handler registration
07217ad 
* Clicking outside the bubble always hides it
* Clicking inside the bubble never hides it

Signed-off-by: Jonas <[email protected]>
@mejo-
fix(Link): Don't open link bubble for anchor links, jump to anchor di…
60c1711 
…rectly

Signed-off-by: Jonas <[email protected]>
@mejo-
fix(LinkBubble): Allow to close link bubble with Escape
a5911b8 
Signed-off-by: Jonas <[email protected]>
@mejo-
fix(LinkBubble): Hide bubble after 100ms delay
aa673ab 
Should allow Cypress to click on it before blur event happens.

Signed-off-by: Jonas <[email protected]>
@mejo-
fix(LinkBubble): Add some padding and border radius
b73181a 
Signed-off-by: Jonas <[email protected]>
@marcoambrosini
Copy link
Member

what do you think @marcoambrosini

I think that there's no need for such button because there's a link element just below it

@mejo- mejo- merged commit a084306 into main Jan 30, 2024
53 checks passed
@mejo- mejo- deleted the feat/link_preview_click branch January 30, 2024 19:41
@mejo- mejo- mentioned this pull request Feb 12, 2024
Closed
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-nextcloud max-nextcloud max-nextcloud approved these changes

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

Assignees
No one assigned
Labels
3. to review enhancement New feature or request
Projects
🖍 Design team
Archived in project
Milestone
Nextcloud 29
5 participants
@mejo- @juliushaertl @marcoambrosini @max-nextcloud @nimishavijay