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

Use node filter to skip nested components when walking #262

Merged
merged 5 commits into from
Aug 28, 2021

Conversation

joshiggins
Copy link
Contributor

For #261, instead of breaking out of the walker use a NodeFilter to reject the top element of a nested component (using same logic if element has got a unicorn:checksum attr).

When used with a tree walker rejecting a node from the filter skips the current node plus any descendants. In context of the refreshEventListeners function it achieves the same thing as before by not traversing a nested component, but continues to walk any elements that might come after it.

@adamghill
Copy link
Owner

Thanks for the PR! Two questions:

  1. I was testing manually with joshiggins@080e0be, but the load button click fired with my old code and with the filter. Is it a browser thing (I'm testing on Firefox currently) or did I misunderstand how to replicate the problem?
  2. Any idea how to write a test for this? I started trying to figure it out in joshiggins@f6c4322, but ran in to an issue where NodeFilter wasn't available and wasn't sure if there was a way to test this outside of an actual browser context?

Thanks!

@joshiggins
Copy link
Contributor Author

Yep problem is replicated correctly in joshiggins@080e0be thanks! I can't get the second load table button to fire at all with the old code...

I tried in FF, chrome and safari to put breakpoint here

It throws on the nested filter component, then if you keep stepping - it breaks out of the walk and on to next init function.

Resume and then in console Unicorn.getComponent("nested.table"), in the actionEvents.click there is an action for the first button element but not the second.

It looks like jsdom has got all the dom traversal stuff in now so should be able to test it all - will have a go at fleshing out the tests for this. Cheers!

@adamghill adamghill merged commit a54cdaf into adamghill:master Aug 28, 2021
@adamghill
Copy link
Owner

@all-contributors add @joshiggins for test, code

@allcontributors
Copy link
Contributor

@adamghill

I've put up a pull request to add @joshiggins! 🎉

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