chore: Ensure class names conform to PascalCase #5716

hussam-i-am · 2025-02-20T16:15:28Z

Closes https://github.com/github/primer/issues/4250

Changelog

New

Changed

Adds selector-class-pattern rule to check casing of class names
Updates classes that did not conform to casing
Wraps focus-visible and :popover-open classes with :global and make them exceptions to the rule

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2025-02-20T16:15:34Z

🦋 Changeset detected

Latest commit: fc925ea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

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

github-actions · 2025-02-20T16:15:56Z

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

hussam-i-am · 2025-02-20T16:16:46Z

packages/react/src/ActionList/ActionList.module.css

-      & > .ItemLabel {
-        font-weight: var(--base-text-weight-semibold);
-      }
-    }


@langermank I did not see any usage of this class anywhere, so I removed it. Can you confirm?

Oh weird, it is used in Item

Sorry, I meant ActionListContent--hasActiveSubItem. I didn't see an explicit reference to it, or a concatenation of hasActiveSubItem

Ah! Yes, that doesn't appear to be used.

Would be cool to setup something that would check for unused CSS

hussam-i-am · 2025-02-20T16:18:20Z

packages/react/src/BaseStyles.module.css

-[tabindex='0']:focus:not(:focus-visible):not(.focus-visible),
-details-dialog:focus:not(:focus-visible):not(.focus-visible) {
+[tabindex='0']:focus:not(:focus-visible):not(:global(.focus-visible)),
+details-dialog:focus:not(:focus-visible):not(:global(.focus-visible)) {


@jonrohan I noticed that these did not have the :global wrapper, which I assume are needed for it to behave as expected.

🤷🏻 I guess so, but this change might need to be verified

This is going in on the next release, anything specific that needs to be validated UI-wise before landing this? @jonrohan @hussam-i-am

Not sure how to recreate this selector in the dom. Maybe drop a focus-visible class on a details-dialog element

Double checked, doesn't look like it breaks anything. We have some duplicate styles for this in primer/css that's already being imported

github-actions · 2025-02-20T16:18:58Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	105.96 KB (-0.14% 🔽)
packages/react/dist/browser.umd.js	106.31 KB (-0.19% 🔽)

hussam-i-am · 2025-02-20T16:19:09Z

packages/react/src/TooltipV2/Tooltip.module.css

-  &.\\:popover-open,
-  &.\\:popover-open::before {
+  /* stylelint-disable-next-line selector-class-pattern */
+  &.\\:popover-open, &:global(.\\:popover-open)::before {


@jonrohan similar to https://github.com/primer/react/pull/5716/files#r1963911493, wrapped these in :global

joshblack

Thanks for taking the time to put this together! Just left one comment but otherwise 100% down to give this a try 👍

joshblack · 2025-02-21T20:20:01Z

packages/react/src/DataTable/Table.tsx

            })}
          />
        ) : null}
        {direction === SortDirection.DESC ? (
          <SortDescIcon
            className={clsx('TableSortIcon', 'TableSortIcon--descending', {
              [classes.TableSortIcon]: enabled,
-              [classes['TableSortIcon--descending']]: enabled,


For this style I think it'd be helpful to preserve the original convention from BEM until we can update it to use our preferred approach with data attributes. Having -descending ends up as a weird hybrid between the two and I think it can lose the modifier meaning because of it.

Makes sense, updated

…am/stylelint-pascal

jonrohan · 2025-03-03T18:40:47Z

packages/react/src/ActionList/ActionList.module.css

-      & > .ItemLabel {
-        font-weight: var(--base-text-weight-semibold);
-      }
-    }


Would be cool to setup something that would check for unused CSS

jonrohan · 2025-03-03T18:41:41Z

packages/react/src/BaseStyles.module.css

-[tabindex='0']:focus:not(:focus-visible):not(.focus-visible),
-details-dialog:focus:not(:focus-visible):not(.focus-visible) {
+[tabindex='0']:focus:not(:focus-visible):not(:global(.focus-visible)),
+details-dialog:focus:not(:focus-visible):not(:global(.focus-visible)) {


🤷🏻 I guess so, but this change might need to be verified

hussam-i-am · 2025-03-03T21:15:58Z

Would be cool to setup something that would check for unused CSS

Yeah, I thought we already had something that checks, but I guess not in primer

…mer/react into hussam-i-am/stylelint-pascal

hussam-i-am · 2025-03-04T15:47:09Z

@jonrohan I added the package typed-css-modules, which will generate type definition files and at least ensure we are using class names that do exist. Couldn't find a clear solution for ensuring that any classes within the css file are being used.

jonrohan

Nice actually found some typos and incorrectly capitalized classes

hussam-i-am added 2 commits February 20, 2025 16:10

chore: Ensure class names conform to PascalCase

2910934

revert lock file

bdf5c06

hussam-i-am self-assigned this Feb 20, 2025

hussam-i-am requested a review from a team as a code owner February 20, 2025 16:15

hussam-i-am requested a review from TylerJDev February 20, 2025 16:15

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 20, 2025

hussam-i-am commented Feb 20, 2025

View reviewed changes

format fix

f1891ea

github-actions bot requested a deployment to storybook-preview-5716 February 20, 2025 16:26 Abandoned

fix lint disables

f629583

hussam-i-am temporarily deployed to github-pages February 20, 2025 16:39 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5716 February 20, 2025 16:40 Inactive

hussam-i-am temporarily deployed to github-pages February 20, 2025 16:53 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5716 February 20, 2025 16:53 Inactive

hussam-i-am temporarily deployed to github-pages February 20, 2025 17:06 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5716 February 20, 2025 17:06 Inactive

joshblack reviewed Feb 21, 2025

View reviewed changes

hussam-i-am added 2 commits February 21, 2025 20:35

Merge branch 'main' of https://github.com/primer/react into hussam-i-…

55eac0e

…am/stylelint-pascal

conform to BEM

99e8df2

hussam-i-am temporarily deployed to github-pages February 21, 2025 20:58 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5716 February 21, 2025 20:58 Inactive

Merge branch 'main' into hussam-i-am/stylelint-pascal

6ae6fbe

jonrohan temporarily deployed to github-pages February 22, 2025 03:24 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5716 February 22, 2025 03:24 Inactive

hussam-i-am requested a review from joshblack February 25, 2025 17:00

hussam-i-am requested a review from jonrohan March 3, 2025 20:03

hussam-i-am disabled auto-merge March 3, 2025 20:04

jonrohan approved these changes Mar 3, 2025

View reviewed changes

langermank approved these changes Mar 3, 2025

View reviewed changes

hussam-i-am added 2 commits March 4, 2025 05:42

Add typed-css-modules

e1ae9b5

Merge branch 'hussam-i-am/stylelint-pascal' of https://github.com/pri…

6f3e6ff

…mer/react into hussam-i-am/stylelint-pascal

hussam-i-am deployed to github-pages March 4, 2025 15:56 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-5716 March 4, 2025 15:56 Inactive

hussam-i-am requested review from jonrohan, langermank and joshblack March 4, 2025 16:01

jonrohan approved these changes Mar 4, 2025

View reviewed changes

add prebuild step to generate files

be9f73f

hussam-i-am deployed to github-pages March 4, 2025 16:58 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-5716 March 4, 2025 16:58 Inactive

limit to src files

4d04814

hussam-i-am deployed to github-pages March 4, 2025 17:46 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-5716 March 4, 2025 17:47 Inactive

hussam-i-am added 2 commits March 4, 2025 18:07

update to postbuild

039d234

remove post built

fc925ea

github-actions bot requested a deployment to storybook-preview-5716 March 4, 2025 18:20 Abandoned

hussam-i-am deployed to github-pages March 4, 2025 18:33 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-5716 March 4, 2025 18:33 View deployment

hussam-i-am added this pull request to the merge queue Mar 4, 2025

Merged via the queue into main with commit d62a494 Mar 4, 2025
44 checks passed

hussam-i-am deleted the hussam-i-am/stylelint-pascal branch March 4, 2025 19:25

primer bot mentioned this pull request Mar 4, 2025

Release tracking #5717

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Ensure class names conform to PascalCase #5716

chore: Ensure class names conform to PascalCase #5716

hussam-i-am commented Feb 20, 2025

changeset-bot bot commented Feb 20, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025

hussam-i-am Feb 20, 2025

langermank Feb 20, 2025

hussam-i-am Feb 20, 2025

langermank Feb 25, 2025

jonrohan Mar 3, 2025

hussam-i-am Feb 20, 2025

jonrohan Mar 3, 2025

francinelucca Mar 5, 2025

jonrohan Mar 5, 2025

jonrohan Mar 5, 2025

github-actions bot commented Feb 20, 2025 •

edited

Loading

hussam-i-am Feb 20, 2025

joshblack left a comment

joshblack Feb 21, 2025

hussam-i-am Feb 21, 2025

jonrohan Mar 3, 2025

jonrohan Mar 3, 2025

hussam-i-am commented Mar 3, 2025

hussam-i-am commented Mar 4, 2025

jonrohan left a comment

chore: Ensure class names conform to PascalCase #5716

chore: Ensure class names conform to PascalCase #5716

Conversation

hussam-i-am commented Feb 20, 2025

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Feb 20, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2025 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

joshblack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hussam-i-am commented Mar 3, 2025

hussam-i-am commented Mar 4, 2025

jonrohan left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Feb 20, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025 •

edited

Loading