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

feat(biome_css_analyzer): implement selector-pseudo-class-no-unknown #3034

Merged

Conversation

tunamaguro
Copy link
Contributor

@github-actions github-actions bot added A-Project Area: project L-CSS Language: CSS A-Diagnostic Area: diagnostocis labels Jun 1, 2024
Copy link

codspeed-hq bot commented Jun 1, 2024

CodSpeed Performance Report

Merging #3034 will not alter performance

Comparing tunamaguro:feat/css-selector-pseudo-class-no-unknown (88d8b3a) with main (38abf5e)

Summary

✅ 92 untouched benchmarks

Comment on lines 72 to 91
fn is_webkit_pseudo_class(node: &AnyPseudoLike) -> bool {
let mut prev_element = node.syntax().parent().and_then(|p| p.prev_sibling());
while let Some(prev) = &prev_element {
let maybe_selector: Option<CssPseudoElementSelector> = prev.clone().cast();
if let Some(selector) = maybe_selector.as_ref() {
return WEBKIT_SCROLLBAR_PSEUDO_ELEMENTS.contains(&selector.text().trim_matches(':'));
};
prev_element = prev.prev_sibling();
}

false
}
Copy link
Contributor Author

@tunamaguro tunamaguro Jun 1, 2024

Choose a reason for hiding this comment

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

To determine whether a ::-webkit-scrollbar CSS pseudo-element has a selector, I'm retrieving each previous element one by one, but I feel there might be a performance issue.

Copy link
Contributor

@togami2864 togami2864 left a comment

Choose a reason for hiding this comment

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

Thank you! I left some suggestions.

Comment on lines 65 to 78
declare_node_union! {
pub AnyPseudoLike = CssPseudoClassFunctionCompoundSelector|CssPseudoClassFunctionCompoundSelectorList|CssPseudoClassFunctionIdentifier
|CssPseudoClassFunctionNth|CssPseudoClassFunctionRelativeSelectorList|CssPseudoClassFunctionSelector
|CssPseudoClassFunctionSelectorList|CssPseudoClassFunctionValueList|CssPseudoClassIdentifier
|CssBogusPseudoClass|CssPageSelectorPseudo
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
declare_node_union! {
pub AnyPseudoLike = CssPseudoClassFunctionCompoundSelector|CssPseudoClassFunctionCompoundSelectorList|CssPseudoClassFunctionIdentifier
|CssPseudoClassFunctionNth|CssPseudoClassFunctionRelativeSelectorList|CssPseudoClassFunctionSelector
|CssPseudoClassFunctionSelectorList|CssPseudoClassFunctionValueList|CssPseudoClassIdentifier
|CssBogusPseudoClass|CssPageSelectorPseudo
}
declare_node_union! {
pub AnyPseudoLike =
CssPseudoClassFunctionCompoundSelector
| CssPseudoClassFunctionCompoundSelectorList
| CssPseudoClassFunctionIdentifier
| CssPseudoClassFunctionNth
| CssPseudoClassFunctionRelativeSelectorList
| CssPseudoClassFunctionSelector
| CssPseudoClassFunctionSelectorList
| CssPseudoClassFunctionValueList
| CssPseudoClassIdentifier
| CssBogusPseudoClass
| CssPageSelectorPseudo
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestions. I fixed it in 8a79444

fn is_webkit_pseudo_class(node: &AnyPseudoLike) -> bool {
let mut prev_element = node.syntax().parent().and_then(|p| p.prev_sibling());
while let Some(prev) = &prev_element {
let maybe_selector: Option<CssPseudoElementSelector> = prev.clone().cast();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let maybe_selector: Option<CssPseudoElementSelector> = prev.clone().cast();
let maybe_selector = CssPseudoElementSelector::cast_ref(prev);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestions. I fixed it in 4eae3f0

a:pseudo-class { }
body:not(div):noot(span) {}
a:unknown::before { }
a,\nb > .foo:error { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
a,\nb > .foo:error { }
a,
b > .foo:error { }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it in 3144105 and update snapshots

div :nth-child(2 of .widget) { }
a:hover::before { }
a:-moz-placeholder { }
a,\nb > .foo:hover { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
a,\nb > .foo:hover { }
a,
b > .foo:hover { }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it in 3144105 and update snapshots

version: "next",
name: "noUnknownPseudoClassSelector",
language: "css",
recommended: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
recommended: false,
recommended: true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it in 83e8e9f

@tunamaguro tunamaguro force-pushed the feat/css-selector-pseudo-class-no-unknown branch from 83e8e9f to 7db746b Compare June 5, 2024 12:18
@tunamaguro
Copy link
Contributor Author

@togami2864
Thank you for the review!
I have finished the corrections. Could you please review it again?

Copy link
Contributor

@togami2864 togami2864 left a comment

Choose a reason for hiding this comment

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

I left one small nit but LGTM

@togami2864 togami2864 merged commit aa2b52f into biomejs:main Jun 9, 2024
12 checks passed
@tunamaguro tunamaguro deleted the feat/css-selector-pseudo-class-no-unknown branch June 9, 2024 21:59
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis A-Project Area: project L-CSS Language: CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Implement selector-pseudo-class-no-unknown
3 participants