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

Remove element.current from useEffect in BubbleMenu and FloatingMenu #2297

Merged
merged 6 commits into from
Dec 22, 2021
Merged

Remove element.current from useEffect in BubbleMenu and FloatingMenu #2297

merged 6 commits into from
Dec 22, 2021

Conversation

ValentaTomas
Copy link
Contributor

@ValentaTomas ValentaTomas commented Dec 19, 2021

Changes to the element.current don't trigger useEffect 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.

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.
@netlify
Copy link

netlify bot commented Dec 19, 2021

✔️ 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

@philippkuehn
Copy link
Contributor

Hey, in this PR you added also a check for element.current. Can this also be removed then?

And could you do the same change to the FloatingMenu too please?

@ValentaTomas
Copy link
Contributor Author

Oh thanks, I completely forgot about the FloatingMenu.

I will also fix the element.current issue properly!

@ValentaTomas ValentaTomas changed the title Remove element.current from useEffect dependencies Remove element.current from useEffect in BubbleMenu and FloatingMenu Dec 20, 2021
@ValentaTomas
Copy link
Contributor Author

ValentaTomas commented Dec 20, 2021

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?

error: pathspec 'patch-1' did not match any file(s) known to git
Error: Invalid status code: 1
    at ChildProcess.<anonymous> (/home/runner/work/_actions/stefanzweifel/git-auto-commit-action/v4/index.js:17:19)
    at ChildProcess.emit (events.js:210:5)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5) {
  code: 1
}
Error: Invalid status code: 1
    at ChildProcess.<anonymous> (/home/runner/work/_actions/stefanzweifel/git-auto-commit-action/v4/index.js:17:19)
    at ChildProcess.emit (events.js:210:5)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5)

@ValentaTomas
Copy link
Contributor Author

Please don't merge it yet, I'm still testing some behavior.

@ValentaTomas
Copy link
Contributor Author

ValentaTomas commented Dec 21, 2021

Ok, BubbleMenu looks ok, but I have not tested FloatingMenu. It think it should be ok, but I really don't want to break anything. Do you have automated tests or some sandbox where you test the features?

@hanspagel
Copy link
Contributor

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)

@ValentaTomas
Copy link
Contributor Author

lint:fix helped - it was just bad formatting.

When I have the FloatingMenu tested I will notify you here.

@ValentaTomas
Copy link
Contributor Author

FloatingMenu seems ok too. I think the PR is ready to merge.

@philippkuehn philippkuehn merged commit 561941d into ueberdosis:main Dec 22, 2021
@philippkuehn
Copy link
Contributor

looks good. thanks!

@hanspagel
Copy link
Contributor

Thanks for your work! 👏

@ValentaTomas
Copy link
Contributor Author

No problem. Thank you for creating TipTap.

@ValentaTomas ValentaTomas deleted the patch-1 branch December 22, 2021 19:53
@ValentaTomas ValentaTomas restored the patch-1 branch December 22, 2021 19:53
@ValentaTomas ValentaTomas deleted the patch-1 branch December 22, 2021 19:53
@ValentaTomas ValentaTomas restored the patch-1 branch December 22, 2021 19:53
@ValentaTomas ValentaTomas deleted the patch-1 branch December 22, 2021 19:53
andrewlu0 pushed a commit to trybaseplate/tiptap that referenced this pull request Oct 20, 2023
…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
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.

3 participants