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

refactor(extension/bubble-menu): add debounce to bubble menu updates #3385

Merged
merged 2 commits into from
Nov 4, 2022

Conversation

bdbch
Copy link
Contributor

@bdbch bdbch commented Nov 4, 2022

This pull requests implements a debounce to the BubbleMenu plugin which allows developers to control how the bubble menu is shown.

It includes the debounce function from lodash which is imported directly without importing the whole lodash package which should keep the bundle size low.

See #1493

Closes #1493
Closes #1313

@bdbch bdbch self-assigned this Nov 4, 2022
@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 386bdc3
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/636528b07c55640009d8619f
😎 Deploy Preview https://deploy-preview-3385--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bdbch bdbch merged commit cd5fd60 into main Nov 4, 2022
@bdbch bdbch deleted the feature/bubble-menu-ux branch November 4, 2022 15:39
@svenadlung
Copy link
Contributor

Pretty nice! I was thinking of maybe changing the name of the option. delay could be a lil confusing and easily mixed up with the tippy option delay. It's not delaying the visibility of the bubble menu but a debounce of the update function. So I would tend to sth. like updateDelay or debounce (which could be true with a default or in ms).

@bdbch
Copy link
Contributor Author

bdbch commented Nov 4, 2022

Good point @svenadlung - I updated this accordingly. The new option is now updateDelay

@rumbcam
Copy link
Contributor

rumbcam commented Jan 31, 2023

I'm curious as to why the debounce is only run if validSelection. I have the same use case as #3633 which was partially fixed with this commit e9d9d88

My use case is that when cursor is on a link, regardless of there being a selection or not, I want to show a bubble menu that has options like unlink or edit link etc.

This is partially working. When I click on the link it still runs the this.updateHandler(view, oldState), but it does so immediately without the debounce. This ultimately calls the shouldShow function that I passed in which is returning the result of editor.isActive('link'). What I find strange though is that when I click on the link that results in false at the time that it is called. However if I make the update debounce even with a time of 0 then my shouldShow function returns true. The current results are when you first click on a link the bubble menu does not show up. Then if you click anywhere else, on the link or not on the link, then the bubble menu shows up. I expected it to show up only when I clicked on the link and not when I clicked on something else.

A possible solution that I can see is to always run the debounce regardless of validSelection and if I want a much smaller debounce I can set the updateDelay to 1. (Still keeping updateDelay 0 as a way to turn off the debounce should be fine).

But ultimately I'm curious what the thought process was to only allow the debounce when text was selected and not just always run it if it is turned on

@rumbcam
Copy link
Contributor

rumbcam commented Jan 31, 2023

After looking into it all today I figured out what's happening. More details on my setup. I'm using Vue 3 and using the useEditor composition from @tiptap/vue-3.

The instance of Editor I get back from that is build on top of the Editor instance from @tiptap/core. One big difference though is that it returns a "reactiveState" object when trying to get editor.state. This object however only updates when the 'transaction' event is emitted, which happens after the bubble menu has its update function called.

the transaction event gets emitted in @tiptap/core in the dispatchTransaction, but it does so right after calling this.view.updateState(state). While prosemirror is running the update state it calls all of it's plugins update function and thus we get into bubble menu's update function which during that calls the shouldShow function. So when using useEditor and bubble menu at the time that you get into the shouldShow function there is a mismatch on state. editor.state !== editor.view.state. Thus calling editor.isActive('link') will not return the correct response because editor has not had it's state updated yet.

The work around I found is to import the function isActive directly from @tiptap/core instead of using the isActive method on editor. Then pass view.state as the first parameter and link as the second

import { isActive } from '@tiptap/core';

function shouldShow({ view }) {
    return isActive(view.state, 'link');
}

andrewlu0 pushed a commit to trybaseplate/tiptap that referenced this pull request Oct 20, 2023
…eberdosis#3385)

* refactor(extension/bubble-menu): add debounce to bubble menu updates

* fix: change default duration in react bubble menu demo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve UX on bubble menu when selecting text Bubble menu doesn't position correctly if image has alignment
3 participants