Fix nth-child bug #6

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

jkroso commented Feb 28, 2013

There was a problem with the way you were doing the nth-child thing.
nth-child filters for elements that are an nth child. e.g:

results.filter(function(el){
    return el.parentElement.children[n] === el
})

this meant that if not all element siblings shared the same classlist
etc.. you would end up with an incorrect selector.

The later commits are just general tidying up and optimisation.

Owner

ericclemmons commented Mar 27, 2013

Released v0.0.2 (b987f00) which fixes this via 504480c.

Because nth-child's index is based on the index of the element amongst it's siblings, any selectors more specific than the tag itself is ignored in the calculation.

Or, in other words, if you have 4 <li> tags, each with a different, completely unique class, no matter how specific your selector is, it will always be the nth-child within the parent node.

Since the tool is meant primarily for taking advantage of semantic structure with reporting & analytics, any result that has :nth-child appended to it will still have any relavant classes attached.

This is extremely useful when you have dynamic classes based on user events, such as selected, active, validation-error, etc. Otherwise, you would just know something happened on :nth-child(47), without seeing any descriptive classes in the selector.

@jkroso Unfortunately, each commit you had was not atomic enough for me to cherry-pick directly. If they were, this would have been merged in a month ago with very little review required. However, since there was changes to the coding style, renaming of variables, modifications to the README and various changes largely inconsequential to the bug, I had to re-implement.

Thanks for the feedback & help guys! Open up a new PR if v0.0.2 creates any issues for you!

Contributor

jkroso commented Mar 27, 2013

OK sorry about the mess.

My use case is quite different, I'm using it in an event delegation library where the output only needs to be correct not readable so I'll probably keep my branch around.

I didn't explain it it the PR because I didn't realise it at the time but appending nth-child to the end isn't enough to ensure the selector is unique in cases where the there are parallel branches in the DOM tree. e.g.

<div>
  <div></div>
</div>
<div>
  <div></div>
<div>

in this case a selector of div > div:nth-child(1) is not unique. Probably not a big deal for your use case but thought I should mention it. I dealt with it by always using nth-child where an ID isn't available because it was easy but you could do better.

Owner

ericclemmons commented Mar 27, 2013

Ah, that makes much more sense. What I imagine it should do is use nth-child on any portion of the selector that's required to make it unique: div:nth-child(1) > div, in that case.

The output now should both be correct and readable, based on the behavior of nth-child basically negating any non-tag selectors on the siblings.

Contributor

jkroso commented Mar 27, 2013

yea that should work! not sure what you mean by nth-child negating non-tag selectors though. it adds to the specificity of the selector so div.selected:nth-child(1) > div is not equal to div:nth-child(1) > div.

Owner

ericclemmons commented Mar 27, 2013

That's what I thought, too, but in Chrome (see my previous explanation), the nth-child selector was indexed relative to the tag & it's siblings, not the matching elements in the selector. That threw me off!

Contributor

jkroso commented Mar 27, 2013

Oh, I think we are just mis-understanding each other. Running the following queries on this page demonstrate nth-child behaviour pretty well though so if you feel the need to check.

document.querySelectorAll('#discussion_bucket > div > div > div.action-bubble:nth-child(1)');
document.querySelectorAll('#discussion_bucket > div > div > div.action-bubble:nth-child(6)');
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment