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

Added support for nth-child(An+b of ...) #18

Merged
merged 4 commits into from
Apr 14, 2021
Merged

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Apr 6, 2021

As specified in https://www.w3.org/TR/selectors-4/#nth-child-pseudo
(and implemented in WebKit)

Parse the subtree after the "of" keyword for nth-child/nth-last-child

As specified in https://www.w3.org/TR/selectors-4/#nth-child-pseudo
(and implemented in WebKit)

Parse the subtree after the "of" keyword for nth-child/nth-last-child
Copy link
Owner

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for working on this!

I do not think we should support parsing this without calculating its specificity properly (especially since parsing works fine now already, it just doesn't have the subtree).
Would you be willing to work on that?

parsel.js Outdated Show resolved Hide resolved
parsel.js Outdated
if (node.type === "pseudo-class" && node.argument) {
if (RECURSIVE_PSEUDO_CLASSES.has(node.name)) {
node.subtree = parse(node.argument, {recursive: true, list: true});
} else if (INDEX_PSEUDO_CLASSES.has(node.name)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is INDEX_PSEUDO_CLASSES supposed to contain? The name implies it's more general, but the code matches a specific microsyntax that applies to :nth-child() and :nth-last-child()

Essentially this is a recursive pseudo-class with additional metadata, I wonder if we just need a data structure for that (e.g. RECURSIVE_PSEUDO_CLASSS_ARG)

Copy link
Contributor Author

@noamr noamr Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't exactly follow what RECURSIVE_PSEUDO_CLASSS_ARG would contain. the /([\dn+-]+)\s+of/ regex?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! With named groups for the additional parameters you want to extract.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@noamr
Copy link
Contributor Author

noamr commented Apr 11, 2021

Hey, thanks for working on this!

I do not think we should support parsing this without calculating its specificity properly (especially since parsing works fine now already, it just doesn't have the subtree).
Would you be willing to work on that?

Sure, let's see what's needed :)

Copy link
Owner

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. See the one minor comment.

Could you add a few tests? Admittedly we don't have nearly enough, but there is a skeleton in the repo where you can see how to add tests.

export const RECURSIVE_PSEUDO_CLASSES_ARGS = {
"nth-child": NTH_CHILD_ARG,
"nth-last-child": NTH_CHILD_ARG
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think it would be better to do RECURSIVE_PSEUDO_CLASSES_ARGS['nth-last-child'] = RECURSIVE_PSEUDO_CLASSES_ARGS['nth-child'] after than to define another constant which looks like it's significant, but is essentially just a helper.

@LeaVerou LeaVerou merged commit c4f6489 into LeaVerou:master Apr 14, 2021
@LeaVerou
Copy link
Owner

Thank you for iterating on this and being open to feedback. Merged!

@noamr
Copy link
Contributor Author

noamr commented Apr 15, 2021

Thank you for iterating on this and being open to feedback. Merged!

My pleasure. Hope to contribute more to this in the future.

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