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

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

@max-nextcloud max-nextcloud force-pushed the fix/hide-bubble-when-in-menu branch 2 times, most recently from 43aedac to 6fe12c9 Compare March 19, 2024 12:41
@max-nextcloud max-nextcloud self-assigned this Mar 19, 2024
mejo-
mejo- approved these changes Mar 19, 2024
View reviewed changes
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.

@max-nextcloud max-nextcloud force-pushed the fix/hide-bubble-when-in-menu branch from d068db7 to dcb7dc6 Compare March 20, 2024 07:27
@max-nextcloud
refactor(links): move linkBubble plugin to plugins/links
710b350 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): pass editor via options to LinkBubblePluginView
e6049b0 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): untangle LinkBubblePluginView from editor
d871277 
It is a prosemirror plugin view. No need for it to know tiptap data structures

Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): use prosemirror tr and state to track clicks
e4340f8 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): move click handling into link bubble plugin
90b99be 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): only hand link mark to updateTooltip
356509b 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): introduce setActiveLink function
e30c78c 
Also rename `clicked` state to `active`.
We will also use it for activating via keypress

Signed-off-by: Max <[email protected]>
@max-nextcloud
fix(links): also update if active was unset
d956fdd 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): operate on state with linkNodeFromSelection
e02c5ad 
In appendTransation callbacks there is no view available.

Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): move linkNodeFromSelection into helper
c1e4bd8 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): handle selection changes in plugin
96097aa 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): use hideLinkBubble command for esc
4a3165b 
Signed-off-by: Max <[email protected]>
@max-nextcloud
fix(links): simplify updateTooltip and handle active null
ed923f4 
Signed-off-by: Max <[email protected]>
@max-nextcloud
fix(links): remove special handling for clicked and focus
4bc7586 
Signed-off-by: Max <[email protected]>
@max-nextcloud
fix(links): handle esc in DOMEvents rather than prop
7fe10f9 
The `handleKeyDown` prop does not work in read only mode.

Signed-off-by: Max <[email protected]>
@max-nextcloud
fix(links) handle linkNodeFromSelection returning false
d79b67c 
Signed-off-by: Max <[email protected]>
@max-nextcloud
enh(links): close link bubble when opening preview toggle
4f5277e 
Signed-off-by: Max <[email protected]>
@max-nextcloud
fix(lint): add missing jsdoc comments to new files
d582f38 
Signed-off-by: Max <[email protected]>
@max-nextcloud
docs(preview): improve jsdocs for markdownit preview plugin
322a752 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): only get mark from selection
8e78cb7 
Signed-off-by: Max <[email protected]>
@max-nextcloud
refactor(links): get active link from selection
385b246 
Signed-off-by: Max <[email protected]>
@max-nextcloud
fix(links): show bubble with cursor on the end of the link
c2fd1e5 
Signed-off-by: Max <[email protected]>
@max-nextcloud
tests(links): ignore link bubble when reading link text
fb0a8ce 
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 <[email protected]>
@max-nextcloud max-nextcloud force-pushed the fix/hide-bubble-when-in-menu branch from dcb7dc6 to fb0a8ce Compare March 20, 2024 09:53
@max-nextcloud max-nextcloud enabled auto-merge March 20, 2024 09:53
@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
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
Merged
@mejo- mejo- mentioned this pull request Mar 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mejo- mejo- mejo- approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

@max-nextcloud max-nextcloud

Labels
3. to review 29 feedback
Projects
📝 Office team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@max-nextcloud @mejo-