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

Add destroy method to TooltipView #41

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

fkling
Copy link
Contributor

@fkling fkling commented Oct 14, 2022

Other components that extend the editor's DOM have a destroy method that allows them to clean up any resources they created (e.g. PluginValue, Panel). I've been missing such a method for the TooltipView as well. Although I found a workaround by using MutationObserver like so

    public mount(): void {
        if (this.dom.parentElement) {
            const observer = new MutationObserver(mutations => {
                for (const mutation of mutations) {
                    if (mutation.type === 'childList' && Array.from(mutation.removedNodes).includes(this.dom)) {
                        // clean up here
                        observer.disconnect()
                        return
                    }
                }
            })
            observer.observe(this.dom.parentElement, { childList: true })
        }
    }

I think it would be nice to have such a method for simplicity and consistency.

Note: I extended one of the hover tests but I wasn't able to actually run web tests via npm run test. I got an error that the @codemirror/view dependency doesn't exist (which makes sense) but I don't know what the necessary steps are to get the tests running.

This commit adds support for a `destroy` method to `TooltipView`.
Similar to other interfaces this method is called when the tooltip is
removed or the editor is destroyed so that it can clean up any resources.
@marijnh marijnh merged commit b6b784d into codemirror:main Oct 14, 2022
@marijnh
Copy link
Member

marijnh commented Oct 14, 2022

This is a good idea. Thanks for the patch!

@fkling
Copy link
Contributor Author

fkling commented Oct 14, 2022

Thank you for accepting it!

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.

2 participants