Follow up for Chaplin#780

[Florian-R]

Not green yet. Still one test failing.

[akre54]

I think the Chaplin style is

children = if itemSelector
    list.querySelectorAll(itemSelector)
  else
    list.children

[Florian-R]

Fixed the coding style but i'm starting to think it wasn't worth push this. I've seen @paulmillr suggestion to throw early just after, but after some investigations, i 'm afraid it might be a breaking change. This jsbin demonstrate visually the behavior on master of a CollectionView with an itemSelector with or without listSelector.

I've double checked with the tests on master with exos setup and this test pass, so it's definitively something supported.

The failure come from qsa which retrieve all the matching nodes, as opposed to the implementation on master which rely on Backbone.utils.matchesSelector (exposed by Exoskeleton) and filter only the directs children.

Maybe we can come up with something like, based on my previous attempt and code from Exos (quickly done, untested and ugly)

matchesSelector: (node, selector) ->
  matches = node.matches || node.webkitMatchesSelector || node.mozMatchesSelector || node.msMatchesSelector

  if !matchesSelector
    throw new Error('Element#matches is not supported');

  matches.call(node, selector)

filterChildren = (nodeList, selector) ->
  return nodeList unless selector
  for node in nodeList when matchesSelector node, selector
    if $ and $(node).is(selector)
      node
    else if matchesSelector.call(node, selector)
      node

[akre54]

Why not prepend the > selector to the qsa to select only child nodes?

list.querySelectorAll("> #{itemSelector}")

[Florian-R]

It could have been a nice solution indeed... But it throws an error in every browsers.

A quick googling seems to indicate there's only very contrived solutions to use querySelectorAll with a child selector.

[akre54]

Ah crap. I remember that. (It's a specific reason why we didn't move off jQuery a few years back, for instance).

Any reason not to use this shim from Backbone.NativeView?

[akre54]

The matches on children vs listSelector isn't ideal, but I can't imagine it would break many if any things.

I'd also be fine dropping IE8 support, since almost every other browser supports matches natively.

[Florian-R]

I'd also be fine dropping IE8 support, since almost every other browser supports matches natively.

I've mixed feeling about that. It will require a major version bump, no?

I'd rather go with the same shim than Backbone.NativeView, minus the prefixed version (excepted msMatchesSelector maybe).

If we go down this route, the remaining question is do we expose matchesSelector in Chaplin.utils (can be useful in a no jQuery world, but i'm unsure it belongs here) or do we keep it private?

[akre54]

I've mixed feeling about that. It will require a major version bump, no?

I'd imagine any changes we make here would require a major version bump.

I'd rather go with the same shim than Backbone.NativeView, minus the prefixed version (excepted msMatchesSelector maybe).

I dunno.... Firefox just dropped the moz prefix 2 versions ago, and webkit (safari and chrome both) weren't that much longer than that. It doesn't hurt anything, and we can always drop them down the line.

I'm fine exposing on Chaplin.utils. Would be easier to override if someone has a better shim (or our implementation doesn't work with their selector on older browsers).

[akre54]

Feel free to make that change and I'll merge

[Florian-R]

Change done, might need small tweaks to fit the coding style

Feel free to make that change and I'll merge

My first idea was to ask you to not merge as is, as i intend to fix the tests with the Exos setup.
But as you've opened #3, maybe it's best to merge this, implement the suggested changes an then go back to fix the tests?

Sidenote. If this get merged, maybe it's a good idea to squash 1c63e3e with the last commit to keep the original PR clean.

[Florian-R]

Implementation is strictly the same than NativeView's one, except i've just slighly tweaked it to match the construct of beget, indexOf etc. Let me know if you prefer to keep it like the original.

[akre54]

In general I'm not a fan of functions wrapping a call just to preserve a signature. I think it's fine if in filterChildren we do utils.matchesSelector.call node, selector instead

[Florian-R]

Should be OK now. Commited separately 969a9e3 so it can be easily reverted / squashed in the original PR.