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_analyze): implement noDuplicateFontNames #2308

Merged

Conversation

togami2864
Copy link
Contributor

Summary

Experimental implement font-family-no-duplicate-names from stylelint and add some predefined keywords, utils.

Test Plan

added tests and snapshots

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Website Area: website L-CSS Language: CSS A-Diagnostic Area: diagnostocis labels Apr 4, 2024
@togami2864 togami2864 self-assigned this Apr 4, 2024
Copy link

netlify bot commented Apr 4, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit bf404c1
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/66169f2f7a4ccd000801035a
😎 Deploy Preview https://deploy-preview-2308--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🟢 up 4 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codspeed-hq bot commented Apr 4, 2024

CodSpeed Performance Report

Merging #2308 will degrade performances by 7.87%

Comparing togami2864:feat/no-font-family-duplicate-names (bf404c1) with main (62e5aab)

Summary

❌ 1 regressions
✅ 92 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main togami2864:feat/no-font-family-duplicate-names Change
eucjp.json[cached] 4 ms 4.4 ms -7.87%

@Conaclos Conaclos changed the title feat(biome_css_analyze): implment NoFontFamilyDuplicateNames feat(biome_css_analyze): implement NoFontFamilyDuplicateNames Apr 4, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The first CSS lint rule!!!! 🎆

/// ```
pub NoFontFamilyDuplicateNames {
version: "next",
name: "noFontFamilyDuplicateNames",
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about noRedundantFontName? We already have some rules that follow this naming convention

Copy link
Member

Choose a reason for hiding this comment

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

Or noDuplicateFontNames to align with our existing rule names?

Copy link
Member

@ematipico ematipico Apr 4, 2024

Choose a reason for hiding this comment

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

Both seem valid, your suggestion is better

Comment on lines 126 to 132
Some(RuleDiagnostic::new(
rule_category!(),
span,
markup! {
"Unexpected duplicate font name: "<Emphasis>{ state.value }</Emphasis>
},
))
Copy link
Member

Choose a reason for hiding this comment

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

Let's follow the rule pillars:

  • show the error
  • explain why it could be an error
  • suggest a solution

The solution usually is the code action, if we don't have a code action, we should suggest something.

///
/// This rule checks the `font` and `font-family` properties for duplicate font names.
///
/// This rule ignores var(--custom-property) variable syntaxes.
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a good excuse to evaluate the implementation of a possible semantic model for CSS :)

Of course, not in this PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it will come true in the near future:) e8510f4

@togami2864 togami2864 force-pushed the feat/no-font-family-duplicate-names branch from af8b357 to e8510f4 Compare April 6, 2024 10:11
];

pub const FONT_WEIGHT_ABSOLUTE_KEYWORDS: [&str; 2] = ["normal", "bold"];
pub const FONT_WIGHT_NUMERIC_KEYWORDS: [&str; 9] = [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub const FONT_WIGHT_NUMERIC_KEYWORDS: [&str; 9] = [
pub const FONT_WEIGHT_NUMERIC_KEYWORDS: [&str; 9] = [

"contextual",
"no-contextual",
"small-caps",
"small-caps",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"small-caps",

@togami2864 togami2864 force-pushed the feat/no-font-family-duplicate-names branch from 748d412 to bf404c1 Compare April 10, 2024 14:16
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

You've done awesome work, @togami2864!

@togami2864 togami2864 changed the title feat(biome_css_analyze): implement NoFontFamilyDuplicateNames feat(biome_css_analyze): implement noDuplicateFontNames Apr 11, 2024
@togami2864 togami2864 merged commit ce223aa into biomejs:main Apr 11, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project A-Website Area: website L-CSS Language: CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants