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

Omit tagName from nth-child selector as it is redundant #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eoghanmurray
Copy link

This is another way of asking the following question which I asked in #3

I'm wondering why #some_id div:nth-child(2) a is preferred over #some_id :nth-child(2) a in the existing code? That div appears to be redundant in the examples I've checked.

@eoghanmurray
Copy link
Author

I'm publishing this PR to see if the automatic tests fail (they won't run for me on my local machine)

@antonmedv
Copy link
Owner

So from the tests. Changes to the next:

- script:nth-child(8)
+ body > :nth-child(8)
- .cell:nth-child(1) > p:nth-child(3)
+ .-gutters > :nth-child(1) > :nth-child(3)

So this change will not make selectors shorter in all cases. Sometimes the selector will become longer.

More interesting solution will be to add support for :nth-of-type.

@eoghanmurray
Copy link
Author

Ah okay, I've got the tests running locally so can have a better play around with it.
I've changed it to only omit tagNames, but the longer selectors in the examples you thrown up by the tests that you highlighted still exist.
I think we'd need to see what :nth-of-type looks like first as the two could interact.

On a more semantic note, I think relying on the intersection of position and tagName, while it might produce a shorter selector is very brittle.

E.g.

<span
<a href
<a href   <-- a:nth-child(3)

If a span is later inserted (in a different version of the page, or as modifications are made to the page), then we are now referencing a different element:

<span
<span class="new
<a href   <-- a:nth-child(3)
<a href   

So applying this PR is maybe a judgement call either in favour of robustness or brevity, would you agree?

@antonmedv
Copy link
Owner

So applying this PR is maybe a judgement call either in favour of robustness or brevity, would you agree?

True. I'm not sure what path is better yet.

One concern I have: imagine selector on

.foo :nth-child(0), later some other tag added. The selector is still valid but points to a wrong element. But .foo div:nth-child(0) will break (better to fail fast).

@eoghanmurray
Copy link
Author

The selector is still valid but points to a wrong element

Yes, but you are acknowledging that redundancy is good. There are plenty of other times when finder doesn't produce the required level of redundancy for the situation (e.g. my example above).

I've some further ideas with which to update this PR, but for now have produced #70 as the next step, so no point in continuing this discussion if that one is unacceptable!

@eoghanmurray
Copy link
Author

I've some parts of nth-of-type already implemented and I'll include it in this PR shortly

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.

None yet

2 participants