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

[BUG] widget destroy() method fails testing. #7460

Open
buggyj opened this issue May 16, 2023 · 5 comments
Open

[BUG] widget destroy() method fails testing. #7460

buggyj opened this issue May 16, 2023 · 5 comments

Comments

@buggyj
Copy link
Contributor

buggyj commented May 16, 2023

Describe the bug

I have written some test scripts to test my destroy plugin (for v5.2.7)

These tests aim to show:

  • when a widget is to be removed its destroy() function is called

  • when a widget's destroy() function is called it is able to access its dom nodes

I have tried these tests with prerelease of 5.3.0 and 2 of the first 3 tests fail (and so the later tests cannot be performed).

The test wiki is here, (includes descriptions of the tests and test widgets):

https://destroytests530prerelease.tiddlyhost.com/

for reference the tests with my plugin (all tests passing) are here:

https://destroytest.tiddlyhost.com/

Expected behavior

No response

To Reproduce

No response

Screenshots

No response

TiddlyWiki Configuration

  • Version [e.g. v5.3.0prerelease]

Additional context

No response

@Jermolene
Copy link
Owner

Hi @buggyj I am not familiar with your "destroy" plugin:

https://github.com/buggyj/destroy/blob/main/common/destroyfunction.js

It looks like a superior implementation of the "destroy" method that was merged in #6699, is that right? Would it be feasible for the core to adopt your implementation?

@buggyj
Copy link
Contributor Author

buggyj commented May 17, 2023

yes of course. I think the code could be simplified a bit further - I will put in a pull request.

@Jermolene
Copy link
Owner

Thank you @buggyj – I've reverted #6699 for the moment

@linonetwo
Copy link
Contributor

I read the code, functionality is basically the same (at least for tw-react plugin), we can use this simpler one for maintainability.

One thing different is I omit the dom removal of child widget https://github.com/Jermolene/TiddlyWiki5/pull/6699/files#diff-923154c1ac214c303ed3a97ac47050febdbea3c32b9b246c53e015b32b2641cdR631

Because when you remove parent dom node, you don't need to remove children. This can save some (not calculated yet) expensive dom operations.

@pmario
Copy link
Contributor

pmario commented May 19, 2023

@buggyj I think this one can be closed now
@linonetwo There should be a new issue handling the .destroy() communication ... or open the old one.

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

No branches or pull requests

4 participants