Skip to content

Commit

Permalink
fix: Remove element.current from useEffect in BubbleMenu and `F…
Browse files Browse the repository at this point in the history
…loatingMenu` (#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
  • Loading branch information
ValentaTomas authored Dec 22, 2021
1 parent fadafae commit 561941d
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 22 deletions.
27 changes: 16 additions & 11 deletions packages/react/src/BubbleMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import React, { useEffect, useRef } from 'react'
import React, {
useEffect, useState,
} from 'react'
import { BubbleMenuPlugin, BubbleMenuPluginProps } from '@tiptap/extension-bubble-menu'

type Optional<T, K extends keyof T> = Pick<Partial<T>, K> & Omit<T, K>
Expand All @@ -8,10 +10,14 @@ export type BubbleMenuProps = Omit<Optional<BubbleMenuPluginProps, 'pluginKey'>,
}

export const BubbleMenu: React.FC<BubbleMenuProps> = props => {
const element = useRef<HTMLDivElement>(null)
const [element, setElement] = useState<HTMLDivElement | null>(null)

useEffect(() => {
if (!element.current) {
if (!element) {
return
}

if (props.editor.isDestroyed) {
return
}

Expand All @@ -22,24 +28,23 @@ export const BubbleMenu: React.FC<BubbleMenuProps> = props => {
shouldShow = null,
} = props

editor.registerPlugin(BubbleMenuPlugin({
const plugin = BubbleMenuPlugin({
pluginKey,
editor,
element: element.current as HTMLElement,
element,
tippyOptions,
shouldShow,
}))
})

return () => {
editor.unregisterPlugin(pluginKey)
}
editor.registerPlugin(plugin)
return () => editor.unregisterPlugin(pluginKey)
}, [
props.editor,
element.current,
element,
])

return (
<div ref={element} className={props.className} style={{ visibility: 'hidden' }}>
<div ref={setElement} className={props.className} style={{ visibility: 'hidden' }}>
{props.children}
</div>
)
Expand Down
27 changes: 16 additions & 11 deletions packages/react/src/FloatingMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import React, { useEffect, useRef } from 'react'
import React, {
useEffect, useState,
} from 'react'
import { FloatingMenuPlugin, FloatingMenuPluginProps } from '@tiptap/extension-floating-menu'

type Optional<T, K extends keyof T> = Pick<Partial<T>, K> & Omit<T, K>
Expand All @@ -8,10 +10,14 @@ export type FloatingMenuProps = Omit<Optional<FloatingMenuPluginProps, 'pluginKe
}

export const FloatingMenu: React.FC<FloatingMenuProps> = props => {
const element = useRef<HTMLDivElement>(null)
const [element, setElement] = useState<HTMLDivElement | null>(null)

useEffect(() => {
if (!element.current) {
if (!element) {
return
}

if (props.editor.isDestroyed) {
return
}

Expand All @@ -22,24 +28,23 @@ export const FloatingMenu: React.FC<FloatingMenuProps> = props => {
shouldShow = null,
} = props

editor.registerPlugin(FloatingMenuPlugin({
const plugin = FloatingMenuPlugin({
pluginKey,
editor,
element: element.current as HTMLElement,
element,
tippyOptions,
shouldShow,
}))
})

return () => {
editor.unregisterPlugin(pluginKey)
}
editor.registerPlugin(plugin)
return () => editor.unregisterPlugin(pluginKey)
}, [
props.editor,
element.current,
element,
])

return (
<div ref={element} className={props.className} style={{ visibility: 'hidden' }}>
<div ref={setElement} className={props.className} style={{ visibility: 'hidden' }}>
{props.children}
</div>
)
Expand Down

0 comments on commit 561941d

Please sign in to comment.