Skip to content

chore(linting): 🤖 Enable linter on CSS files#945

Open
punkbit wants to merge 17 commits intochore/init-css-modules-setupfrom
chore/enable-linter-on-css-files
Open

chore(linting): 🤖 Enable linter on CSS files#945
punkbit wants to merge 17 commits intochore/init-css-modules-setupfrom
chore/enable-linter-on-css-files

Conversation

@punkbit
Copy link
Collaborator

@punkbit punkbit commented Mar 24, 2026

Why?

Previously, whether using Styled-Components or our new CSS Modules, there were no safeguards to prevent the use of CSS features unsupported by major browsers. As part of our migration to CSS Modules, this PR introduces CSS linting to the project. This ensures all new styles adhere to browser compatibility standards and maintain consistent linting rules.

During the transition from Styled-Components, the process of porting styles can lead to confusion between migration related adjustments and actual browser support issues. Introducing the linter now provides clarity, ensuring that any compatibility errors are caught automatically rather than being mistaken for migration/porting errors/issues.

⚠️ Depends on #810 (should be merged first)

How?

  • Uses stylelint-config-standard as base
  • Introduces browserlist and filters out obscure browsers while supporting modern ones (Chrome 100+,
    Safari 15.5+, Firefox 99+)
  • stylelint-no-unsupported-browser-features for browser compatibility, e.g. errors on unsupported features
  • Excludes auto-generated token files
  • Updates package.json scripts, e.g. clean separation of code (ESLint) and style (Stylelint) linting
  • Add property order css linter rule
  • Exclude obscure browser support from the list
  • Update documentation

Preview?

N/A

@changeset-bot
Copy link

changeset-bot bot commented Mar 24, 2026

⚠️ No Changeset found

Latest commit: adb7dc8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@punkbit punkbit changed the title chore(linting): 🤖 enable linter on css files chore(linting): 🤖 Enable linter on CSS files Mar 24, 2026
@github-actions
Copy link

github-actions bot commented Mar 24, 2026

Claude finished @punkbit's task in 1m 23s —— View job


Code Review

Solid tooling addition — the config is well-structured, the BEM pattern is correct, the justification comments in stylelint.config.js (css-nesting transpiled, :focus-visible fallback, etc.) are excellent for maintainability. A couple of real issues to address:


🚨 Must Fix

  • .browserslistrc lines 2–3 — floating rolling window: last 4 years and > 0.5% are separate OR-unioned queries, creating a non-deterministic, ever-changing browser set. last 4 years today reaches back to 2022 browsers; a year from now it shifts forward, potentially breaking CSS rules that pass today. The PR description explicitly states the intent is Chrome 100+, Safari 15.5+, Firefox 99+ — but those floors aren't actually enforced here. Consider replacing with pinned minimums to match the stated intent:
    chrome >= 100
    safari >= 15.5
    firefox >= 99
    not and_qq > 0
    not baidu > 0
    not and_uc > 0
    not kaios > 0
    not op_mini all
    
    Fix this →

♿ Accessibility

  • Button.module.css lines 254–258 — prefers-reduced-motion only covers animation, not transitions: The @media (prefers-reduced-motion: reduce) block disables the shimmer animation but leaves transition: var(--click-transition-default) active on all four :hover:not(:disabled) states (lines 49, 86, 123, 160). Users with motion sensitivity will still experience hover transitions. Consider adding transition: none for all hover selectors inside that block. Fix this →

💡 Suggestions (non-blocking)

  • Button.module.css lines 49, 86, 123, 160 — transition on :hover instead of base: Setting transition inside :hover selectors means the state change animates in but snaps back instantly on mouse-out. Standard practice places transition on the base .button rule so both directions animate smoothly. Unless the snap-back is intentional, this is worth moving to the base rule.

  • Button.module.css line 14 (and hover/active/disabled states) — font shorthand: font: var(...) resets all sub-properties (style, variant, weight, stretch, size, line-height, family) to the token value or browser defaults. Verify the token fully specifies all relevant sub-properties; otherwise states that only change e.g. font-weight could silently reset line-height or font-family.


Great work overall — the tooling infrastructure is solid and the PR description is thorough. 🙌

@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 24, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 24, 2026
@punkbit punkbit requested a review from hoorayimhelping March 24, 2026 15:41
@github-actions
Copy link

github-actions bot commented Mar 24, 2026

Claude finished @punkbit's task in 1m 29s —— View job


Code Review

Solid tooling setup — the stylelint.config.js is well-organized, the inline // NOTE: justifications for ignored browser features are great for maintainability, and the BEM selector pattern is correct. A few issues from the previous review are still open, plus one new regression introduced by this push:


🚨 Must Fix

  • .browserslistrc lines 2–3 — floating rolling window (unresolved from previous review): last 4 years and > 0.5% are OR-unioned and non-deterministic. Today "last 4 years" reaches back to 2022; next year it shifts, silently widening the supported set and potentially passing CSS that fails on browsers you didn't intend to support. The PR description states the intent is Chrome 100+, Safari 15.5+, Firefox 99+ — but those floors aren't enforced. Replace with pinned minimums:

    chrome >= 100
    safari >= 15.5
    firefox >= 99
    not and_qq > 0
    not baidu > 0
    not and_uc > 0
    not kaios > 0
    not op_mini all
    

    Fix this →

  • Button.module.csstransition moved to :hover, removing the base transition (regression): The previous code had transition: font var(--click-transition-default) on the base .button rule. This push moved transition: var(--click-transition-default) into each :hover:not(:disabled) selector (lines 49, 86, 123, 160). This creates asymmetric animation: hover-in animates, hover-out snaps back instantly. Standard practice is to place transition on the base rule so both directions animate. Unless the snap-back is intentional, this should be moved back to .button {}.
    Fix this →


♿ Accessibility

  • Button.module.css lines 254–258 — prefers-reduced-motion only covers animation (unresolved from previous review): The @media (prefers-reduced-motion: reduce) block disables the shimmer animation but the transition on all four :hover:not(:disabled) selectors remains active. Users with motion sensitivity will still see hover transitions. Add transition: none inside the prefers-reduced-motion block for all hover selectors.
    Fix this →

💡 Suggestions (non-blocking)

  • Button.module.cssfont shorthand resets all sub-properties: font: var(...) on :hover, :active, :disabled states resets every font sub-property (style, variant, weight, stretch, size, line-height, family) to whatever the token provides or browser defaults. If the token doesn't fully specify all sub-properties, you could silently lose line-height or font-family in certain states. Worth verifying the token covers all sub-properties, or consider switching to just font-weight (or whichever property actually changes per state).

@github-actions
Copy link

github-actions bot commented Mar 24, 2026

Claude finished @punkbit's task in 1m 7s —— View job


Code Review

The tooling setup is clean and well-structured — the inline // NOTE: justifications in stylelint.config.js are great for long-term maintainability. Three issues from previous reviews remain unresolved, flagged below.


🚨 Must Fix

  • .browserslistrc lines 2–3 — floating rolling window (flagged twice before, still unresolved): last 4 years and > 0.5% are OR-unioned and non-deterministic. The README table documents Chrome 100+, Edge 100+, Firefox 99+, Safari 15.5+ as minimums, but the actual config doesn't enforce those floors — last 4 years today reaches back to 2022 and will silently shift forward over time. Replace with pinned minimums to match the documented intent:

    chrome >= 100
    edge >= 100
    safari >= 15.5
    firefox >= 99
    not and_qq > 0
    not baidu > 0
    not and_uc > 0
    not kaios > 0
    not op_mini all
    

    Fix this →

  • Button.module.css lines 49, 86, 123, 160 — transition on :hover creates asymmetric animation (regression, flagged in previous review): transition is set inside each :hover:not(:disabled) rule, so hover-in animates smoothly but hover-out snaps back instantly. Move transition: var(--click-transition-default) to the base .button {} rule for symmetric animation in both directions.
    Fix this →


♿ Accessibility

  • Button.module.css lines 254–258 — prefers-reduced-motion doesn't suppress transition (flagged in previous review): The @media (prefers-reduced-motion: reduce) block only disables the shimmer animation. The transition on all four :hover:not(:disabled) selectors remains active, so users with motion sensitivity still get hover transitions. Once transition is moved to .button {}, add transition: none inside the reduced-motion block to cover it.
    Fix this →

💡 Suggestions (non-blocking)

  • stylelint.config.js line 114 — BEM selector pattern: DreaminDani flagged this as a potential blocker if the BEM RFC isn't approved first. Punkbit agreed to remove it if needed. Worth a quick decision before merge to avoid locking in a convention that might be revised.

  • Button.module.cssfont shorthand on state selectors: font: var(...) on :hover, :active, :disabled, and :focus-visible resets every font sub-property (weight, style, size, line-height, family) to whatever the token resolves to. If any token only specifies a subset of sub-properties, you may silently lose line-height or font-family in those states. Worth verifying the tokens fully specify all sub-properties, or switching to font-weight / the specific property that actually changes.


@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-8zrbvzu1y-clickhouse.vercel.app

Built from commit: 81e0838c2c5c8abb348afae312c7ebf877f3d766

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.

2 participants