-
Notifications
You must be signed in to change notification settings - Fork 10
Add namespace-spacing-utils rule #490
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
base: main
Are you sure you want to change the base?
Conversation
|
Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
| '<div className={`pr-m-4 pr-p-2`} />', | ||
|
|
||
| // Responsive variants (already namespaced) | ||
| '<div className="md:pr-m-4" />', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot This is the responsive format: .mx-sm-2 and .mx-md-4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in aa07187. Updated the regex pattern and tests to use the Primer CSS responsive format (mx-sm-2, mx-md-4, etc.) instead of the colon-separated format.
Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new ESLint rule (namespace-spacing-utils) to enforce pr- namespacing for Primer CSS spacing utility classes (margin/padding), including responsive and negative/auto variants, with autofix support.
Changes:
- Added the
namespace-spacing-utilsrule implementation for detecting un-namespaced spacing utility classes inclassName. - Added a comprehensive RuleTester suite for valid/invalid cases and autofix expectations.
- Added rule documentation and exported the rule from the plugin entrypoint.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/rules/namespace-spacing-utils.js | Implements detection + autofix for un-namespaced spacing utility classes. |
| src/rules/tests/namespace-spacing-utils.test.js | Adds test coverage for detection and fixes across many spacing patterns. |
| src/index.js | Registers the new rule in the plugin’s exported rules map. |
| docs/rules/namespace-spacing-utils.md | Documents the rule’s intent, patterns, and autofix behavior. |
|
|
||
| ## When to use autofix | ||
|
|
||
| This rule includes an autofix that will automatically add the `pr-` prefix to unnamespaced spacing utility classes. The autofix is safe to use as it only modifies the class names that match the spacing utility patterns. |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section states the autofix is “safe to use”, but the current fixer implementation operates via substring replacement on the raw node text, which can rewrite non-spacing class names that contain m-*/p-* substrings, and may not fix multiple different spacing classes in one pass due to overlapping fixes. After adjusting the fixer to be boundary-aware and non-overlapping, update this wording (or qualify it) so the docs match the actual behavior.
| This rule includes an autofix that will automatically add the `pr-` prefix to unnamespaced spacing utility classes. The autofix is safe to use as it only modifies the class names that match the spacing utility patterns. | |
| This rule includes an autofix that will automatically add the `pr-` prefix to unnamespaced spacing utility classes. The autofix operates on individual class tokens and is designed to only modify class names that match the spacing utility patterns, but you should still review the changes before committing. |
| fix(fixer) { | ||
| // Get the raw text of the value node | ||
| const rawText = sourceCode.getText(valueNode) | ||
|
|
||
| // Replace the unnamespaced class with the namespaced version | ||
| // Using replaceAll to handle multiple occurrences of the same class | ||
| const fixedText = rawText.replaceAll(original, replacement) | ||
| return fixer.replaceText(valueNode, fixedText) |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autofix currently does a plain rawText.replaceAll(original, replacement) on the entire Literal/TemplateElement text. This can incorrectly rewrite other class names that merely contain the substring (e.g. fixing m-4 would also change my-m-4), and it also makes every report replace the whole node so fixes overlap and ESLint will apply at most one of them. Consider generating a single fixed string using a boundary-aware regex (or tokenizing on whitespace) and returning the same full-node fix for every report (or reporting once per node) so --fix reliably namespaces all matching classes without touching unrelated substrings.
| // p-{size}, px-{size}, py-{size}, pt-{size}, pr-{size}, pb-{size}, pl-{size} | ||
| // Sizes: 0-12, auto, n1-n12 (negative) | ||
| // Responsive variants: mx-sm-2, mx-md-4, etc. (breakpoint embedded in class name) | ||
| const spacingUtilPattern = /(?<!\S)([mp][xytblr]?-(?:sm-|md-|lg-|xl-)?(?:auto|n?[0-9]+))(?!\S)/g |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacingUtilPattern currently matches any numeric size (n?[0-9]+), but the rule/docs comment say the supported scale is 0-12 and n1-n12. As-is, this will flag/auto-fix non-Primer classes like m-99. Tighten the regex to the intended range (including negative) to avoid false positives.
| const spacingUtilPattern = /(?<!\S)([mp][xytblr]?-(?:sm-|md-|lg-|xl-)?(?:auto|n?[0-9]+))(?!\S)/g | |
| const spacingUtilPattern = /(?<!\S)([mp][xytblr]?-(?:sm-|md-|lg-|xl-)?(?:auto|[0-9]|1[0-2]|n(?:[1-9]|1[0-2])))(?!\S)/g |
| // Multiple unnamespaced classes - each is reported and fixed (first pass fixes first match only in test) | ||
| { | ||
| code: '<div className="m-4 p-2" />', | ||
| output: '<div className="pr-m-4 p-2" />', | ||
| errors: [ | ||
| {messageId: 'namespaceRequired', data: {className: 'm-4', replacement: 'pr-m-4'}}, | ||
| {messageId: 'namespaceRequired', data: {className: 'p-2', replacement: 'pr-p-2'}}, | ||
| ], | ||
| }, |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case’s output only applies the first fix (m-4), even though the rule reports (and should be able to fix) both m-4 and p-2 in a single --fix run. Once the rule’s fixer is made non-overlapping (e.g. produce a single full replacement per node), update the expected output to include both prefixes, and consider adding a regression test that ensures fixing m-4 does not rewrite my-m-4 or other non-token substrings.
| 'enforce-button-for-link-with-no-href': require('./rules/enforce-button-for-link-with-no-href'), | ||
| 'enforce-css-module-default-import': require('./rules/enforce-css-module-default-import'), | ||
| 'enforce-css-module-identifier-casing': require('./rules/enforce-css-module-identifier-casing'), | ||
| 'namespace-spacing-utils': require('./rules/namespace-spacing-utils'), |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI enforces the presence of a Changeset file (see .github/workflows/check-for-changeset.yml). Since this PR adds a new user-facing rule, it likely needs a changeset in .changeset/*.md (or the PR must be labeled skip changeset) so the workflow doesn’t fail.
| # Enforce namespacing of spacing utility classes (namespace-spacing-utils) | ||
|
|
||
| Primer CSS spacing utility classes (margin and padding) should be namespaced with the `pr-` prefix to avoid conflicts with other CSS frameworks and ensure consistent styling. |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule is documented here, but it isn’t currently listed in the top-level README.md “Rules” section (which enumerates all existing rules). Add namespace-spacing-utils to that list so users can discover the new rule and its docs.
Summary: This PR adds a new ESLint rule
namespace-spacing-utilsthat enforces namespacing of Primer CSS spacing utility classes (margin/padding) with thepr-prefix.The rule:
m-4,mx-2,my-3,mt-1, etc.p-4,px-2,py-3,pt-1, etc.m-n4,mt-n2autovalues likem-auto,mx-automx-sm-2,mx-md-4,p-lg-3(Primer CSS format)pr-prefix automaticallyExample transformations:
m-4→pr-m-4p-2→pr-p-2mx-md-4→pr-mx-md-4