Add namespaced styles to primers spacing #2962
Conversation
🦋 Changeset detectedLatest commit: 6c939ad The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
adierkens
temporarily deployed
to
github-pages
GitHub Actions
Inactive
siddharthkp
left a comment
siddharthkp
left a comment
There was a problem hiding this comment.
Approved on behalf of engineer-reviewers. Will wait for @lukasoppermann to approve for design
Co-authored-by: Siddharth Kshetrapal <[email protected]>
adierkens
temporarily deployed
to
github-pages
GitHub Actions
Inactive
adierkens
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| } | ||
|
|
||
| .m-auto { margin: auto !important; } | ||
| .m-auto { |
There was a problem hiding this comment.
@adierkens We probably want to namespace auto as well because of the additional !important
There was a problem hiding this comment.
Can you elaborate on what you mean by the additional !important? All the primer/css util classes have important, but as long as the fundamental definitions don't conflict I think we're okay to leave it non-namespaced for now.
The only big difference I can see is the difference between the mx-auto definitions, which may merit updating (primer uses margin-left/right and tailwind leverages -inline and -block)
There was a problem hiding this comment.
All the primer/css util classes have important, but as long as the fundamental definitions don't conflict I think we're okay to leave it non-namespaced for now.
Fair. The reason I wanted to namespace the whole spacing scale is so that when you mix custom css with tailwind utilities, you know what you are getting. Having a !important primer className would overwrite the tailwind classname. This isn't just a spacing problem of course, it's with all utils with the same name.
primer uses margin-left/right and tailwind leverages -inline and -block
I think that would be fine because afaik we don't officially support RTL layout.
There was a problem hiding this comment.
Pull request overview
This PR adds pr- prefixed namespace versions to all margin and padding utility classes to enable transitions to Tailwind-compatible sizing while maintaining backward compatibility.
Changes:
- Added
pr-namespaced versions for all padding utilities (p, pt, pr, pb, pl, px, py) - Added
pr-namespaced versions for margin utilities (m, mt, mb, mr, ml, mx, my) including negative margins - Reformatted auto margin utilities for consistency
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/utilities/padding.scss | Added pr- prefixed versions for all padding utility classes while preserving original classes |
| src/utilities/margin.scss | Added pr- prefixed versions for margin utility classes and reformatted auto margin classes |
| .changeset/empty-islands-push.md | Documents the addition of pr- namespace for spacing utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .m-auto { | ||
| margin: auto !important; | ||
| } | ||
|
|
||
| .mt-auto { | ||
| margin-top: auto !important; | ||
| } | ||
|
|
||
| .mt-auto { margin-top: auto !important; } | ||
| .mr-auto { margin-right: auto !important; } | ||
| .mb-auto { margin-bottom: auto !important; } | ||
| .ml-auto { margin-left: auto !important; } | ||
| .mr-auto { | ||
| margin-right: auto !important; | ||
| } | ||
|
|
||
| .mb-auto { | ||
| margin-bottom: auto !important; | ||
| } | ||
|
|
||
| .ml-auto { | ||
| margin-left: auto !important; | ||
| } |
There was a problem hiding this comment.
The auto margin utilities are missing their pr- namespaced versions. For consistency with the rest of the changes in this PR, these utilities should also have namespaced versions added. The following classes need namespaced versions:
.m-auto→.pr-m-auto.mt-auto→.pr-mt-auto.mr-auto→.pr-mr-auto.mb-auto→.pr-mb-auto.ml-auto→.pr-ml-auto
See below for a potential fix:
.m-auto,
.pr-m-auto {
margin: auto !important;
}
.mt-auto,
.pr-mt-auto {
margin-top: auto !important;
}
.mr-auto,
.pr-mr-auto {
margin-right: auto !important;
}
.mb-auto,
.pr-mb-auto {
margin-bottom: auto !important;
}
.ml-auto,
.pr-ml-auto {
What are you trying to accomplish?
Prefix all
m-andp-util css with a new namespace to enable transitions to tailwind compatible sizingCan these changes ship as is?