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 itemSelector in internalCount initialization #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

net02
Copy link

@net02 net02 commented May 3, 2016

Fixes a bug with an incorrect internalCount when the html does not have collection items as direct children of the collection, i.e:

<table data-form-widget="collection">
    <thead>...</thead>
    <tbody class="items">
        <tr class="item">...</tr>
        <tr class="item">...</tr>
    </tbody>
</table>

@merk
Copy link
Contributor

merk commented May 3, 2016

Please add a test case for this change, otherwise looks good.

@net02
Copy link
Author

net02 commented May 4, 2016

@merk done and witnessed the test turning green from red as I reapplied the internalCount initialization fix :)

@merk
Copy link
Contributor

merk commented May 4, 2016

Talking about this change in our office we realise that it is going to affect nested collections, which we dont seem to have any tests for in the JS tests. We'll have to fix that.

In the mean time, you can work around this issue by setting the data-form-widget on the tbody instead of the table, which is what we do for form collections as table rows.

Fixes a bug with an incorrect internalCount when the html does not have collection items as direct children of the collection, i.e:

<table data-form-widget="collection">
    <thead>...</thead>
    <tbody class="items">
        <tr class="item">...</tr>
        <tr class="item">...</tr>
    </tbody>
</table>
@net02
Copy link
Author

net02 commented May 6, 2016

@merk you were absolutely right, having a nested collection would have messed up the internalCount badly..

I fiddled up with jQuery selectors a bit and came up with the current setup which keeps track of the level of nesting (with the same item selector) and excludes nested collections' items.
There's also a new test for it :)

@merk
Copy link
Contributor

merk commented May 7, 2016

The issue here is if someone wanted to customise the itemSelector for nested collections. I'm not sure theres going to be a neat solution to the issue unless we set some kind of requirement that the item selector must be exact for nested collections like '> tbody > .item' (and I have no idea if jquery will support such a query in a .find() call)

@net02
Copy link
Author

net02 commented May 7, 2016

Either I am missing something in the big picture or you missed the latest test which actually has a different itemSelector on one level of the "collection tree".

Having a different selector actually simplifies things because there wouldn't be any "false positives" in the internalCount .find() of the parent collection.
This being the case, the .not() still prevents any item of an N-th level child collection, with the same selector as one of the parents, being included.

The nestedSelector loop thingy keeps the .not() from excluding true positives.. it's really an how-many-levels-of-nesting-with-the-same-selector-am-i-in count, which works for both "every nested collection has ss the same selector", "every nested collection has its own selector" scenarios, and also "mixed selectors at some level of nesting" (like the test case) or "someone put the whole collection under one or more .item parents which we really don't care".

Could you please show me what I am missing, maybe with a red test?

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