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): noUnknownSelectorPseudoElement #2655

Merged

Conversation

keita-hino
Copy link
Contributor

Summary

close #2624
Implement selector-pseudo-element-no-unknown

Please note that the following is not yet addressed:

  • Implementation of options
  • Handling extended CSS language cases such as sass and less

Test Plan

added tests and snapshots

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

codspeed-hq bot commented Apr 30, 2024

CodSpeed Performance Report

Merging #2655 will not alter performance

Comparing keita-hino:feat/no-unknown-selector-pseudo-element (808cc37) with main (150dd0e)

Summary

✅ 88 untouched benchmarks

@keita-hino keita-hino marked this pull request as ready for review April 30, 2024 10:00
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!

Comment on lines 131 to 148
"backdrop",
"content",
"cue",
"file-selector-button",
"grammar-error",
"highlight",
"marker",
"placeholder",
"selection",
"shadow",
"slotted",
"spelling-error",
"target-text",
"view-transition",
"view-transition-group",
"view-transition-image-pair",
"view-transition-new",
"view-transition-old",
Copy link
Contributor

Choose a reason for hiding this comment

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

These values should be defined in keyword.rs

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!
I defined it in keyword.rs with the following commit.
a9b4e0d

a::PSEUDO { }
a::element { }
a:hover::element { }
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 addressed it in the following commit.
48b651f

Comment on lines 116 to 124
pub fn vender_prefix(prop: &str) -> String {
let re = Regex::new(r"^(-\w+-)").unwrap();
let vendor_prefix = re
.captures(prop)
.map(|caps| caps[0].to_string())
.unwrap_or_default();

vendor_prefix
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need Regex since the case is really simple and `Re crate is expensive. We can just define the prefixes in an ordered array, and check against them using the binary search.

Copy link
Contributor Author

@keita-hino keita-hino May 1, 2024

Choose a reason for hiding this comment

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

Indeed, it seems like it can be implemented without using Regex. Thank you!
The -moz-test in the test case also needs to be extracted as a vendor prefix. Since I understand that binary_search cannot perform partial matches, I replaced it with starts_with for the correction.
b0892bd

@keita-hino
Copy link
Contributor Author

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


// Returns the vendor prefix extracted from an input string.
pub fn vender_prefix(prop: &str) -> String {
let prefixes = ["-webkit-", "-moz-", "-ms-", "-o-"];
Copy link
Contributor

Choose a reason for hiding this comment

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

These prefixes should move to keyword.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I'm sorry.
I fixed it with the following commit.
808cc37

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.

Awesome work!

@togami2864 togami2864 merged commit afa5004 into biomejs:main May 2, 2024
12 checks passed
@keita-hino keita-hino deleted the feat/no-unknown-selector-pseudo-element branch May 2, 2024 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-element-no-unknown
2 participants