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.

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

Fix/hide bubble when in menu #5529

Merged
merged 23 commits into from
Mar 20, 2024
Merged

Fix/hide bubble when in menu #5529

merged 23 commits into from
Mar 20, 2024

Conversation

max-nextcloud
Copy link
Collaborator

📝 Summary

  • See: Link preview follow up #5442

  • Move the event handling into the LinkBubble plugin and expose a command to hide the link bubble.

  • Make use of that command to hide the link bubble when opening the preview menu.

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • [Tests] lower level implementation of the hide command is covered with unit test. Tested the ui by hand in various scenarios (write / read only, firefox / chromium)
  • Documentation is not required

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Really nice work 🤩

I went through all the commits and think I followed most of the code changes. Lots of nice refactoring and cleanups in there!

I also did some manual testing around the link bubble and everything seems work reliable there. Just one nitpicking comment.

src/plugins/LinkBubblePluginView.js Outdated Show resolved Hide resolved
@mejo-
Copy link
Member

mejo- commented Mar 19, 2024

Not sure whether the failing Cypress test "Link website with selection" from Links.spec.js is related though.

@max-nextcloud
Copy link
Collaborator Author

Not sure whether the failing Cypress test "Link website with selection" from Links.spec.js is related though.

It probably is. I think the link happens in the a tag but behind the text. It then triggers both the click handler and the selection update. The selection update will remove the link bubble again as the cursor is behind the link not inside it.

I'm tending to make the selection handling more inclusive - that is also show the link bubble when the cursor is in front of the link or behind the link as these cursor positions can be reached by clicking the link.

Signed-off-by: Max <max@nextcloud.com>
It is a prosemirror plugin view. No need for it to know tiptap data structures

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Also rename `clicked` state to `active`.
We will also use it for activating via keypress

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
In appendTransation callbacks there is no view available.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
The `handleKeyDown` prop does not work in read only mode.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
The link bubble now also gets rendered
when the cursor is behind the link.

`cy.get` will always start at the root of the doc.
So it also found the link inside the link bubble.
Use `.find` on the content instead as the link bubble is outside of that.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud merged commit 5dfe6a6 into main Mar 20, 2024
59 checks passed
@max-nextcloud max-nextcloud deleted the fix/hide-bubble-when-in-menu branch March 20, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants