search

brianmhunt / knockout-fast-foreach

Foreach. Faster. For Knockout.
MIT License
59 stars 18 forks source link

Add ability to reuse DOM nodes when array items move, closes #33 #34

Closed cervengoc closed 8 years ago

cervengoc commented 9 years ago

With this feature, on every array operation which results moving of several items FastForEach will detect these moves and will not remove/recreate the corresponding DOM structure, but simply move that too.

Scenarios where this is extremely useful:

cervengoc commented 9 years ago

Some tests fail because it seems like on circleci I cannot change timeout. 2 of the tests have a longer run time when testing the moves with very large array. I will fix that with reducing those sizes.

brianmhunt commented 8 years ago

I love these changes – I hope my comments are helpful. Thanks, @cervengoc

brianmhunt commented 8 years ago

I think I hit all the comments – but let me know if I missed something. :) Thanks @cervengoc

cervengoc commented 8 years ago

@brianmhunt OK, thanks for the comments, I think I'm finished now, please check it out. I decided to add a little improvement by adding the possibility to pass a specific DOM node as template. You can see that I used it at the refactored tests, but when I was thinking about it I thought there can also be real life scenarios when it's useful, for example when programatically applying bindings, etc. Hope you don't mind that few characters :)

cervengoc commented 8 years ago

@brianmhunt Does it matter for testing? :) I'm still using scripts for a wider browser support, that's why I wrote script element by instinct.

brianmhunt commented 8 years ago

It definitely shouldn't matter – though it'd be good to try both, just to be sure...

cervengoc commented 8 years ago

@brianmhunt I've changed one of the two tests to use template element, and veryfied the tests. Hope it can be finally ready soon, I have less and less time as Christmas gets closer in time.

brianmhunt commented 8 years ago

Awesome, thanks @cervengoc ! Will look at it ASAP.

krnlde commented 8 years ago

Any news here?

cervengoc commented 8 years ago

This was merged already if I remember correctly.

krnlde commented 8 years ago

Thanks. But why is it still open, any issues left?

brianmhunt commented 8 years ago

Strange — I am not presented with the option to close it.

brianmhunt commented 8 years ago

... it looks like because it's a pull request that's been merged it's been automatically removed from the list of open Pull Requests (which are 0).

cervengoc commented 8 years ago

No issues as far as I know, and I think it's closed (I see 0 pull requests). But if you see any weirdness, ping brianmhunt please.

brianmhunt commented 8 years ago

Incidentally, @cervengoc I've added you as a collaborator on this project so you should be able to make any changes in my absence if needed. :)