-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove element.current
from useEffect
in BubbleMenu
and FloatingMenu
#2297
Conversation
Changes to the `element.current` don't trigger `useEffect` rerender and shouldn't be used in the dependency array. One discussion about is this is for example here: https://stackoverflow.com/questions/60476155/is-it-safe-to-use-ref-current-as-useeffects-dependency-when-ref-points-to-a-dom It's also causing some subtle bugs when mounting and unmounting editors.
✔️ Deploy Preview for tiptap-embed ready! 🔨 Explore the source changes: 2c56915 🔍 Inspect the deploy log: https://app.netlify.com/sites/tiptap-embed/deploys/61c1ea472ffed20008aa5f06 😎 Browse the preview: https://deploy-preview-2297--tiptap-embed.netlify.app |
Hey, in this PR you added also a check for And could you do the same change to the |
Oh thanks, I completely forgot about the I will also fix the |
element.current
from useEffect
dependencieselement.current
from useEffect
in BubbleMenu
and FloatingMenu
This should be it. I have some problems with "build / lint (16) (pull_request) Failing after 1m" though - there is an error in the "Commit fixed linting errors" check. @philippkuehn any idea?
|
Please don't merge it yet, I'm still testing some behavior. |
Ok, |
Thanks! The linting check is broken, but if it would pass you wouldn’t see an exception. You could probably run yarn lint locally to see if there’s something off. (yarn run lint:fix can help fixing linting issues, but not for all rules) |
When I have the |
…`HTMLElement` reference handling
|
looks good. thanks! |
Thanks for your work! 👏 |
No problem. Thank you for creating TipTap. |
…loatingMenu` (ueberdosis#2297) * Remove `element.current` from `useEffect` dependencies Changes to the `element.current` don't trigger `useEffect` rerender and shouldn't be used in the dependency array. One discussion about is this is for example here: https://stackoverflow.com/questions/60476155/is-it-safe-to-use-ref-current-as-useeffects-dependency-when-ref-points-to-a-dom It's also causing some subtle bugs when mounting and unmounting editors. * Fix `FloatingMenu` and `BubbleMenu` element references * Fix linting errors * Don't register plugin when the editor is already destroyed; Simplify `HTMLElement` reference handling * Fix lint error
Changes to the
element.current
don't triggeruseEffect
rerender and shouldn't be used in the dependency array.One discussion about this is for example here: https://stackoverflow.com/questions/60476155/is-it-safe-to-use-ref-current-as-useeffects-dependency-when-ref-points-to-a-dom
It's also causing some subtle bugs when mounting and unmounting editors.