Add tmp- prefix to margin and padding utility classes#3997
Merged
Conversation
🦋 Changeset detectedLatest commit: bdd5d35 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the generated Primer utility-class mapping (lib/primer/classify/utilities.yml) so that margin and padding utilities include an additional tmp- prefixed classname alias, and adjusts the rake task that generates that YAML.
Changes:
- Update
utilities:buildgeneration to emit"#{selector} tmp-#{selector}"for margin/padding selectors. - Regenerate
lib/primer/classify/utilities.ymlsom*/p*entries include both the original andtmp-classnames.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/tasks/utilities.rake | Changes generation logic to append tmp- aliases for margin/padding selectors. |
| lib/primer/classify/utilities.yml | Regenerated mappings now store two classnames in a single string for margin/padding utilities. |
- Update find_selector to handle space-separated class strings - Update utilities_test to expect new output format - Update css_coverage_test to exclude tmp- classes from coverage check
The find_selector method now iterates through space-separated class strings for tmp- namespaced margin/padding utilities, which increases allocations. This is an expected trade-off for the namespacing feature.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces tmp--prefixed companion class names for margin/padding utilities and updates the classify utilities lookup + tests to accommodate utilities.yml entries that now contain space-separated class lists.
Changes:
- Emit
"#{selector} tmp-#{selector}"for margin/padding selectors when generatinglib/primer/classify/utilities.yml. - Update
Primer::Classify::Utilities.find_selector(and CSS coverage test setup) to handle space-separated class strings in the utilities data. - Adjust tests/benchmarks to expect the extra
tmp-classes and updated allocation counts.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/tasks/utilities.rake |
Generates space-separated "selector tmp-selector" values for m*/p* utilities. |
lib/primer/classify/utilities.yml |
Updates margin/padding entries to include tmp- companion classes. |
lib/primer/classify/utilities.rb |
Updates selector lookup to search within space-separated class strings. |
test/performance/bench_utilities.rb |
Raises expected allocation counts for selector validation benchmarks. |
test/lib/css_coverage_test.rb |
Splits space-separated utilities entries and filters out tmp- classes. |
test/lib/classify/utilities_test.rb |
Updates expected classname output to include tmp- classes. |
test/lib/classify_test.rb |
Updates expected generated classes for margin/padding to include tmp- variants. |
test/components/base_component_test.rb |
Updates rendered class assertions for system arguments including tmp- classes. |
Comment on lines
8
to
13
| Primer::Classify::Utilities::UTILITIES | ||
| .flat_map { |_, values| values.flat_map { |_, v| v } } | ||
| .flat_map { |k| k&.split || [] } # Handle space-separated class lists | ||
| .reject { |k| k.start_with?("tmp-") } # Exclude namespaced tmp- classes | ||
| .map { |k| ".#{k}" } | ||
| .uniq |
Move the spacing prefix logic from utilities.rake (YAML generation) to Classify.call (output time). This approach: - Keeps YAML clean with one selector per entry - Preserves original lookup/validation performance - Uses gsub with backreference for efficient string replacement - Adds ~10 allocations to Classify.call (7→17) vs ~11-18 to find_selector The prefix pattern matches margin/padding classes like m-4, px-2, mt-n3, including responsive variants (m-sm-4, etc).
Address review comment: The test named 'with_no_whitespace' should assert the exact class attribute string, not just check that both classes exist.
siddharthkp
commented
Mar 18, 2026
Version 0.9.x has stricter rules that fail on pre-existing ERB issues in the codebase. Pin to 0.8.10 to unblock this PR.
afa643e to
9aab76a
Compare
siddharthkp
commented
Mar 18, 2026
jonrohan
reviewed
Mar 19, 2026
| def bench_allocations | ||
| assert_allocations "4.0" => 7, "3.4" => 7, "3.3" => 7, "3.2" => 6, "3.1" => 6, "3.0" => 6, "2.7" => 4 do | ||
| # Allocations increased due to gsub for tmp- prefixed spacing utilities | ||
| assert_allocations "4.0" => 17, "3.4" => 17, "3.3" => 17, "3.2" => 16, "3.1" => 16, "3.0" => 16, "2.7" => 14 do |
Member
There was a problem hiding this comment.
oof these allocation increases 🤔 will need to think about this maybe there's a better solution
Replace regex-based gsub on output string with inline key check. This reduces allocation overhead from +10 to +3 over baseline. - Use frozen Set of spacing keys for O(1) lookup - Append tmp- prefix inline when key matches - Remove SPACING_CLASS_PATTERN regex constant
Use TMP_PREFIX_CACHE to avoid string allocation during classify calls. Reduces allocations from +3 to +1 over baseline (8 total, was 7).
67a3027 to
bdd5d35
Compare
siddharthkp
commented
Mar 20, 2026
| def bench_allocations | ||
| assert_allocations "4.0" => 7, "3.4" => 7, "3.3" => 7, "3.2" => 6, "3.1" => 6, "3.0" => 6, "2.7" => 4 do | ||
| # +1 allocation from tmp- prefixed spacing utilities (pre-computed cache lookup) | ||
| assert_allocations "4.0" => 8, "3.4" => 8, "3.3" => 8, "3.2" => 7, "3.1" => 7, "3.0" => 7, "2.7" => 5 do |
Member
Author
There was a problem hiding this comment.
@jonrohan prompted some caching, still +1 higher than before. Is this acceptable or should I aim to keep them unchanged?
jonrohan
approved these changes
Mar 23, 2026
| def bench_allocations | ||
| assert_allocations "4.0" => 7, "3.4" => 7, "3.3" => 7, "3.2" => 6, "3.1" => 6, "3.0" => 6, "2.7" => 4 do | ||
| # +1 allocation from tmp- prefixed spacing utilities (pre-computed cache lookup) | ||
| assert_allocations "4.0" => 8, "3.4" => 8, "3.3" => 8, "3.2" => 7, "3.1" => 7, "3.0" => 7, "2.7" => 5 do |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
Towards
Adds a
tmp-prefixed duplicate class for all margin and padding utilities. This will help us drop primercss from dotcom. Keeping both classes so that we don't break styles for non-dotcom appsmb: 3now outputsclass="mb-3 tmp-mb-3"Eventually, once the migration work to drop primercss is complete, we will no longer need to namespace classes. mb: 3 will render
.mb-3which will correspond to the.mb-3present in the app (tailwindcss in our case).For example, to preserve 16px margin, we would need to change mb:3 to mb:4 at the callsite
Risk Assessment
Merge checklist