Added spacing utility classes. #105
Conversation
📝 WalkthroughWalkthroughAdds a new SCSS spacing utility generator and public spacing variables (default scale and a flag for using !important). Also tightens the SCSS dollar-variable naming rule in stylelint. The generator emits margin/padding classes across axes/directions and responsive breakpoints using merged size maps and ct-breakpoint. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Vars as Variables
participant Gen as spacing.scss
participant Mixin as ct-breakpoint()
participant CSS as Generated CSS
Vars->>Gen: Provide $ct-spacing-sizes, $ct-spacing-sizes-default, $ct-spacing-use-important
Gen->>Gen: Merge defaults + overrides into sizes map
loop each breakpoint (including xxs)
alt xxs (base)
Gen->>CSS: Emit base .ct-{prop}{dir}-{size} classes (m/p, t/r/b/l/x/y)
else responsive breakpoint
Gen->>Mixin: Wrap rules with ct-breakpoint(bp)
Mixin-->>CSS: Emit .ct-{prop}{dir}-{bp}-{size} classes
end
end
note right of CSS: Class names include optional !important when configured
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (2)
web/themes/custom/drevops/components/00-base/spacing/spacing.scss(1 hunks)web/themes/custom/drevops/components/variables.components.scss(1 hunks)
🔇 Additional comments (2)
web/themes/custom/drevops/components/variables.components.scss (1)
61-111: LGTM on consistency with existing breakpoint tokens.The new spacing size map uses the same breakpoint keys pattern as nearby config (e.g., Divider map uses 'xxs', 's', 'm', 'l'), so naming is consistent across the theme.
web/themes/custom/drevops/components/00-base/spacing/spacing.scss (1)
56-64: LGTM on class emission and!importanttoggle.Class naming, axis handling, and the conditional
!importantsuffixing are correct and align with typical utility systems.Also applies to: 82-90
| // | ||
| // Spacing component stories. | ||
| // | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Header comment: rename to reflect utilities, not stories.
Minor clarity nit.
-// Spacing component stories.
+// Spacing utilities generator.🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/00-base/spacing/spacing.scss around
lines 1 to 4 the header comment currently reads "// Spacing component stories."
which is misleading; change the comment to indicate this file contains spacing
utilities (e.g., "// Spacing utilities."). Update the header text only to
reflect “utilities” instead of “stories” to improve clarity.
| @use 'sass:string'; | ||
| @use 'sass:list'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused Sass module import.
sass:string isn’t used. Keeping only what’s needed speeds up compilation and avoids lint warnings.
-@use 'sass:string';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @use 'sass:string'; | |
| @use 'sass:list'; | |
| @use 'sass:list'; |
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/00-base/spacing/spacing.scss around
lines 6 to 7, the file currently @use 'sass:string' but that module is not
referenced anywhere; remove the unused @use 'sass:string' line (leaving @use
'sass:list') and run the linter/SCSS build to confirm no references remain and
that compilation passes.
| @use 'sass:map'; | ||
| @use 'sass:string'; | ||
| @use 'sass:list'; | ||
| @import '../../../../../contrib/civictheme/components/00-base/spacing/spacing'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider migrating from @import to @use for Civictheme imports.
@import is deprecated in Dart Sass. If Civictheme exposes modules via @forward, prefer:
@use '../../../../../contrib/civictheme/components/00-base/spacing/spacing' as ct-spacing;and reference mixins via the namespace (e.g., @include ct-spacing.breakpoint(...)). If the dependency doesn’t support @use, keep as-is.
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/00-base/spacing/spacing.scss around line
8, the file uses the deprecated Sass @import for the Civictheme spacing module;
replace the @import with an @use import if Civictheme exposes the module via
@forward (e.g., import it as a namespace like ct-spacing), then update any
mixin/function/variable references in this file to be prefixed with that
namespace (for example change bare mixin includes to use the ct-spacing
namespace). If the Civictheme dependency does not support @use/@forward, leave
the @import as-is. Ensure you run a quick build/check to confirm no unresolved
references remain after namespacing.
| @use 'sass:list'; | ||
| @import '../../../../../contrib/civictheme/components/00-base/spacing/spacing'; | ||
|
|
||
| $_all-sizes: map.merge($ct-spacing-sizes-default, $ct-spacing-sizes); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deep-merge size maps to support partial overrides.
map.merge() is shallow; overriding a single token within a breakpoint requires redefining the whole sub-map. Use a deep merge helper so $ct-spacing-sizes can override only the keys it needs.
Apply this diff (adds a small helper then replaces the merge):
+// Recursively deep-merge two maps. Values from $b override $a.
+@function _ct-map-deep-merge($a, $b) {
+ @if type-of($a) != 'map' or type-of($b) != 'map' {
+ @return $b;
+ }
+ $result: $a;
+ @each $key, $b-val in $b {
+ $a-val: if(map.has-key($a, $key), map.get($a, $key), null);
+ $merged: if(type-of($a-val) == 'map' and type-of($b-val) == 'map', _ct-map-deep-merge($a-val, $b-val), $b-val);
+ $result: map.set($result, $key, $merged);
+ }
+ @return $result;
+}
-
-$_all-sizes: map.merge($ct-spacing-sizes-default, $ct-spacing-sizes);
+$_all-sizes: _ct-map-deep-merge($ct-spacing-sizes-default, $ct-spacing-sizes);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $_all-sizes: map.merge($ct-spacing-sizes-default, $ct-spacing-sizes); | |
| // Recursively deep-merge two maps. Values from $b override $a. | |
| @function _ct-map-deep-merge($a, $b) { | |
| @if type-of($a) != 'map' or type-of($b) != 'map' { | |
| @return $b; | |
| } | |
| $result: $a; | |
| @each $key, $b-val in $b { | |
| $a-val: if(map.has-key($a, $key), map.get($a, $key), null); | |
| $merged: if( | |
| type-of($a-val) == 'map' and type-of($b-val) == 'map', | |
| _ct-map-deep-merge($a-val, $b-val), | |
| $b-val | |
| ); | |
| $result: map.set($result, $key, $merged); | |
| } | |
| @return $result; | |
| } | |
| $_all-sizes: _ct-map-deep-merge($ct-spacing-sizes-default, $ct-spacing-sizes); |
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/00-base/spacing/spacing.scss around line
10, the code uses map.merge($_ct-spacing-sizes-default, $ct-spacing-sizes) which
performs a shallow merge so nested maps (per-breakpoint size maps) are replaced
rather than partially overridden; add a small deep-merge helper function (e.g.,
recursively iterate map keys and merge nested maps) and replace map.merge with
that deep-merge helper so $ct-spacing-sizes can override only specific nested
keys without redefining entire sub-maps.
| // ct-p-0 - padding 0 | ||
| // ct-m-1 - margin 1 | ||
| // ct-p-xs-2 - padding 2 on extra small screens and up | ||
| // ct-mx-xl-2 - margin x-axis (left/right) 2 on extra large screens and up | ||
| // ct-mb-m-1 - margin-bottom 1 on medium screens and up | ||
| // ct-mt-m-1 - margin-top 1 on medium screens and up |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Correct docs: breakpoint token and examples.
Examples reference xs but the code uses xxs as the base (no prefix) and s|m|l|xl for responsive. Update examples to avoid confusion.
-// ct-p-xs-2 - padding 2 on extra small screens and up
+// ct-p-s-2 - padding 2 on small screens and up📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ct-p-0 - padding 0 | |
| // ct-m-1 - margin 1 | |
| // ct-p-xs-2 - padding 2 on extra small screens and up | |
| // ct-mx-xl-2 - margin x-axis (left/right) 2 on extra large screens and up | |
| // ct-mb-m-1 - margin-bottom 1 on medium screens and up | |
| // ct-mt-m-1 - margin-top 1 on medium screens and up | |
| // ct-p-0 - padding 0 | |
| // ct-m-1 - margin 1 | |
| // ct-p-s-2 - padding 2 on small screens and up | |
| // ct-mx-xl-2 - margin x-axis (left/right) 2 on extra large screens and up | |
| // ct-mb-m-1 - margin-bottom 1 on medium screens and up | |
| // ct-mt-m-1 - margin-top 1 on medium screens and up |
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/00-base/spacing/spacing.scss around
lines 19 to 24, the documentation examples incorrectly reference the `xs`
breakpoint while the code uses `xxs` as the base (no prefix) and responsive
tokens `s`, `m`, `l`, `xl`; update the comment examples to use `xxs` (or no
prefix) for base padding/margin and show responsive examples using `s|m|l|xl`
(e.g., ct-p-0 or ct-p-xxs-2 for base, and ct-p-s-2, ct-m-m-1, ct-mx-xl-2 for
responsive) so the examples match the implemented breakpoint tokens.
| // Handle x and y axis spacing | ||
| $axis: list.nth($dir-value, 1); | ||
| $dir1: list.nth($dir-value, 2); | ||
| $dir2: list.nth($dir-value, 3); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused $axis variable.
It’s assigned but not used.
- // Handle x and y axis spacing
- $axis: list.nth($dir-value, 1);
- $dir1: list.nth($dir-value, 2);
- $dir2: list.nth($dir-value, 3);
+ // Handle x and y axis spacing
+ $dir1: list.nth($dir-value, 1);
+ $dir2: list.nth($dir-value, 2);Also applies to: 78-81
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/00-base/spacing/spacing.scss around
lines 51-55 and 78-81, the variable $axis is assigned from list.nth($dir-value,
1) but never used; remove the unused $axis assignments in both locations and
ensure any remaining logic references only $dir1 and $dir2 (or rename/recompute
those if needed) so no dead variables remain.
| @include ct-breakpoint($breakpoint-name) { | ||
| @each $size-name, $size-value in $breakpoint-sizes { | ||
| @each $prop-key, $prop-name in $_spacing-properties { | ||
| @each $dir-key, $dir-value in $_spacing-directions { | ||
| @if $dir-key == 'x' or $dir-key == 'y' { | ||
| // Handle x and y axis spacing | ||
| $axis: list.nth($dir-value, 1); | ||
| $dir1: list.nth($dir-value, 2); | ||
| $dir2: list.nth($dir-value, 3); | ||
|
|
||
| .ct-#{$prop-key}#{$dir-key}-#{$breakpoint-name}-#{$size-name} { | ||
| #{$prop-name}#{$dir1}: $size-value#{if($ct-spacing-use-important, ' !important', '')}; | ||
| #{$prop-name}#{$dir2}: $size-value#{if($ct-spacing-use-important, ' !important', '')}; | ||
| } | ||
| } | ||
| @else { | ||
| .ct-#{$prop-key}#{$dir-key}-#{$breakpoint-name}-#{$size-name} { | ||
| #{$prop-name}#{$dir-value}: $size-value#{if($ct-spacing-use-important, ' !important', '')}; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard against unknown breakpoint keys (optional).
If $ct-spacing-sizes introduces keys not supported by ct-breakpoint, compilation will fail. Consider adding a check with @if function-exists('ct-breakpoint') plus a breakpoint registry match (if available) and @warn when keys are unknown. If such registry isn’t exposed, document that keys must match Civictheme breakpoints.
| $ct-spacing-sizes: () !default; | ||
| $ct-spacing-sizes-default: ( | ||
| 'xxs': ( | ||
| 0: 0, | ||
| 1: ct-particle(1), | ||
| 2: ct-particle(2), | ||
| 3: ct-particle(3), | ||
| 4: ct-particle(4), | ||
| 5: ct-particle(5), | ||
| 'auto': unset, | ||
| ), | ||
| 's': ( | ||
| 0: 0, | ||
| 1: ct-particle(2), | ||
| 2: ct-particle(4), | ||
| 3: ct-particle(6), | ||
| 4: ct-particle(8), | ||
| 5: ct-particle(10), | ||
| 'auto': unset, | ||
| ), | ||
| 'm': ( | ||
| 0: 0, | ||
| 1: ct-particle(3), | ||
| 2: ct-particle(6), | ||
| 3: ct-particle(9), | ||
| 4: ct-particle(12), | ||
| 5: ct-particle(15), | ||
| 'auto': unset, | ||
| ), | ||
| 'l': ( | ||
| 0: 0, | ||
| 1: ct-particle(4), | ||
| 2: ct-particle(8), | ||
| 3: ct-particle(12), | ||
| 4: ct-particle(16), | ||
| 5: ct-particle(20), | ||
| 'auto': unset, | ||
| ), | ||
| 'xl': ( | ||
| 0: 0, | ||
| 1: ct-particle(5), | ||
| 2: ct-particle(10), | ||
| 3: ct-particle(15), | ||
| 4: ct-particle(20), | ||
| 5: ct-particle(25), | ||
| 'auto': unset, | ||
| ), | ||
| ) !default; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify override behavior: shallow merge will replace entire breakpoint maps.
Downstream overrides in $ct-spacing-sizes for a single size (e.g., only key 3 under 'm') will be ignored unless the entire 'm' map is provided, because map.merge() is shallow. If this is intentional, document it. Otherwise, switch to a deep-merge strategy in the generator. See the proposed change in spacing.scss.
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/variables.components.scss around lines 9
to 56, the default $ct-spacing-sizes is defined as nested maps but downstream
overrides will be lost because a shallow map.merge() will replace entire
breakpoint maps; either document this shallow-merge behavior clearly in this
file's comments or (preferred) update the spacing generator to perform a deep
merge: implement or import a recursive map-merge utility that merges nested maps
key-by-key (merging inner maps for each breakpoint and preserving defaults where
overrides omit keys), replace the current shallow map.merge() call with this
deep-merge function, and add a short comment describing the new merge semantics
so consumers know that partial overrides (e.g., just key 3 under 'm') will now
override only that key.
| 3: ct-particle(3), | ||
| 4: ct-particle(4), | ||
| 5: ct-particle(5), | ||
| 'auto': unset, |
There was a problem hiding this comment.
Fix semantic value for the 'auto' size: use auto instead of unset.
margin: unset resets to the initial value (0), not centering like margin: auto. Current utilities would generate .ct-m[-*]-auto that do nothing useful. Replace all 'auto': unset with 'auto': auto.
- 'auto': unset,
+ 'auto': auto,Also applies to: 27-27, 36-36, 45-45, 54-54
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/variables.components.scss around lines
18, 27, 36, 45 and 54, the semantic size value for 'auto' is incorrectly set to
`unset`; replace each `'auto': unset` with `'auto': auto` so generated utilities
like `.ct-m[-*]-auto` produce the intended CSS `margin: auto` behavior instead
of resetting to initial.
| ) !default; | ||
|
|
||
| // Configuration flag to control if !important will be used with spacing utilities. | ||
| $ct-spacing-use-important: true !default; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Reconsider defaulting to !important globally.
Utilities with !important can be hard to override and may increase specificity battles. If this mirrors existing project conventions (e.g., Bootstrap-like utilities), fine; otherwise consider defaulting to false and enabling per-project.
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/variables.components.scss around line 59
the global default sets $ct-spacing-use-important: true !default; which forces
!important on spacing utilities; change the default to false (e.g.
$ct-spacing-use-important: false !default;) so utilities are not globally
!important, and update any places that rely on the previous default to
explicitly enable the flag per-project or per-override; also add or update a
short comment above the variable explaining the intended usage and how to
override it in a project to re-enable !important when needed.
AlexSkrypnyk
force-pushed
the
feature/add-spacing-utilities
branch
from
d577aa9 to
f92e5f7
Compare
AlexSkrypnyk
force-pushed
the
feature/add-spacing-utilities
branch
from
f92e5f7 to
eeb94e7
Compare
AlexSkrypnyk
added
the
AUTOMERGE
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (13)
web/themes/custom/drevops/components/variables.components.scss (3)
9-56: Document override semantics (shallow vs deep merge) next to the size maps.As implemented in spacing.scss, a shallow
map.mergewill replace entire breakpoint maps. Either:
- document that
$ct-spacing-sizesmust redefine the whole sub-map to change one token, or- switch the generator to a deep-merge (recommended; see spacing.scss comment).
If keeping shallow merge, add a short note here:
// Note: Overrides in $ct-spacing-sizes replace entire per-breakpoint maps (shallow merge). // To override a single token, redefine the whole map for that breakpoint.
18-18: Fix semantic value for 'auto' — should beauto, notunset.
margin: unsetresets to initial (often 0) and won’t center. Useautoso utilities like.ct-mx-autowork.Apply:
- 'auto': unset, + 'auto': auto,Also applies to: 27-27, 36-36, 45-45, 54-54
59-59: Defaulting to!importantcan hinder composition. Consider default false.Global
!importanton utilities can create specificity battles. Unless mandated by theme conventions, preferfalseand allow projects to enable when needed.Apply:
-$ct-spacing-use-important: true !default; +$ct-spacing-use-important: false !default;web/themes/custom/drevops/components/00-base/spacing/spacing.scss (10)
8-8: Prefer@useover deprecated@importif Civictheme supports it.If Civictheme forwards its modules, use namespaced imports (e.g.,
@use ... as ct-spacing;) and prefix mixins like@include ct-spacing.breakpoint(...). If not supported, keep@import.Please confirm whether Civictheme exposes
components/00-base/spacing/spacingvia@forward. If yes, I can provide a complete namespaced diff.
2-2: Rename header to reflect utilities, not stories.Minor clarity tweak.
Apply:
-// Spacing component stories. +// Spacing utilities generator.
6-7: Remove unused Sass module import.
sass:stringisn’t referenced.Apply:
@use 'sass:map'; -@use 'sass:string'; @use 'sass:list';
10-10: Deep-merge spacing maps to support partial overrides.
map.mergeis shallow; overriding a single token forces redefining the whole map. Add a small recursive merge and replace the call.Apply:
+// Recursively deep-merge two maps. Values from $b override $a. +@function _ct-map-deep-merge($a, $b) { + @if type-of($a) != 'map' or type-of($b) != 'map' { + @return $b; + } + $result: $a; + @each $key, $b-val in $b { + $a-val: if(map.has-key($a, $key), map.get($a, $key), null); + $merged: if(type-of($a-val) == 'map' and type-of($b-val) == 'map', _ct-map-deep-merge($a-val, $b-val), $b-val); + $result: map.set($result, $key, $merged); + } + @return $result; +} - -$_all-sizes: map.merge($ct-spacing-sizes-default, $ct-spacing-sizes); +$_all-sizes: _ct-map-deep-merge($ct-spacing-sizes-default, $ct-spacing-sizes);
19-24: Fix examples to match actual breakpoint tokens.You use
xxsas the base (no prefix in class names) ands|m|l|xlfor responsive. Avoidxsin examples.Apply:
-// ct-p-0 - padding 0 -// ct-m-1 - margin 1 -// ct-p-xs-2 - padding 2 on extra small screens and up +// ct-p-0 - padding 0 (base) +// ct-m-1 - margin 1 (base) +// ct-p-s-2 - padding 2 on small screens and up
33-41: Simplify direction map and drop the unused axis token.The first element in
('x', ...)/('y', ...)isn’t used. Store only the two directions and adjust consumers.Apply:
$_spacing-directions: ( '': '', 't': '-top', 'r': '-right', 'b': '-bottom', 'l': '-left', - 'x': ('x', '-left', '-right'), - 'y': ('y', '-top', '-bottom') + 'x': ('-left', '-right'), + 'y': ('-top', '-bottom') );And update usage below (see next comments).
51-55: Remove unused$axisand index from updated tuples.Only
$dir1and$dir2are needed.Apply:
- // Handle x and y axis spacing - $axis: list.nth($dir-value, 1); - $dir1: list.nth($dir-value, 2); - $dir2: list.nth($dir-value, 3); + // Handle x and y axis spacing + $dir1: list.nth($dir-value, 1); + $dir2: list.nth($dir-value, 2);
78-81: Remove unused$axisin responsive section too.Mirror the base section change.
Apply:
- // Handle x and y axis spacing - $axis: list.nth($dir-value, 1); - $dir1: list.nth($dir-value, 2); - $dir2: list.nth($dir-value, 3); + // Handle x and y axis spacing + $dir1: list.nth($dir-value, 1); + $dir2: list.nth($dir-value, 2);
47-66: Do not generate invalidpadding: autoutilities.
autois valid for margin but invalid for padding. Guard generation.Apply:
@@ - @each $size-name, $size-value in $breakpoint-sizes { + @each $size-name, $size-value in $breakpoint-sizes { @each $prop-key, $prop-name in $_spacing-properties { - @each $dir-key, $dir-value in $_spacing-directions { + // Skip invalid padding auto utilities. + @if not ($prop-key == 'p' and $size-name == 'auto') { + @each $dir-key, $dir-value in $_spacing-directions { @if $dir-key == 'x' or $dir-key == 'y' { @@ - } - } + } + } + } } } @@ - @each $size-name, $size-value in $breakpoint-sizes { + @each $size-name, $size-value in $breakpoint-sizes { @each $prop-key, $prop-name in $_spacing-properties { - @each $dir-key, $dir-value in $_spacing-directions { + // Skip invalid padding auto utilities. + @if not ($prop-key == 'p' and $size-name == 'auto') { + @each $dir-key, $dir-value in $_spacing-directions { @if $dir-key == 'x' or $dir-key == 'y' { @@ - } - } + } + } + } } }Also applies to: 73-91
72-95: Optional: warn on unknown breakpoint keys.If
$ct-spacing-sizesintroduces keys unmatched byct-breakpoint, Sass will error. Consider a guard (if Civictheme exposes a registry) or document that keys must align with Civictheme breakpoints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (3)
web/themes/custom/drevops/.stylelintrc.json(1 hunks)web/themes/custom/drevops/components/00-base/spacing/spacing.scss(1 hunks)web/themes/custom/drevops/components/variables.components.scss(1 hunks)
🔇 Additional comments (3)
web/themes/custom/drevops/.stylelintrc.json (2)
46-46: Anchoring the variable pattern is a solid tightening.End-anchoring removes accidental substring matches (e.g., "rootcolor"). Good step toward consistent naming.
46-46: No legacy global SCSS variables detected—pattern safe to merge
- A repository-wide scan for SCSS variable declarations not matching
ct-,_…, orrootonly surfaced$axis,$dir1, and$dir2in
web/themes/custom/drevops/components/00-base/spacing/spacing.scss—all inside@each/@includeblocks.
Those are treated as local by theignore: "local"option and won’t be flagged (github.com).- All true global variables (e.g.
$_all-sizes,$_spacing-properties,$_spacing-directions, and any$ct-…constants) already conform to the stricter pattern.No further changes required.
web/themes/custom/drevops/components/00-base/spacing/spacing.scss (1)
10-12: Ensure spacing variables are imported before the spacing generatorThe spacing generator in
•web/themes/custom/drevops/components/00-base/spacing/spacing.scss(line 10)
merges$ct-spacing-sizes-defaultand$ct-spacing-sizes.Those variables are defined in
•web/themes/custom/drevops/components/variables.components.scss(lines 9–12):$ct-spacing-sizes: () !default; $ct-spacing-sizes-default: ( 'xxs': ( 0: 0, … ) );▶ Please confirm that your primary Sass entrypoint (for example,
style.scss,_theme.scss, or equivalent) imports@import 'components/variables.components';before
@import 'components/00-base/spacing/spacing';If you rely on wildcard/glob imports, adjust the load order so that
variables.components.scssalways precedes the spacing generator.
| "scss/comment-no-empty": null, | ||
| "scss/comment-no-loud": true, | ||
| "scss/dollar-variable-pattern": ["^(root|_?ct-.+)", {"ignore": "local"}], | ||
| "scss/dollar-variable-pattern": ["^(root|_.+|ct-.+)$", {"ignore": "local"}], |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Restrict allowed characters for ct-/private variables to kebab-case.
Current .+ will allow uppercase and other symbols. Align with your mixin rule and typical SCSS conventions.
Apply:
-"scss/dollar-variable-pattern": ["^(root|_.+|ct-.+)$", {"ignore": "local"}],
+"scss/dollar-variable-pattern": ["^(root|_[a-z0-9\\-]+|ct-[a-z0-9\\-]+)$", {"ignore": "local"}],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "scss/dollar-variable-pattern": ["^(root|_.+|ct-.+)$", {"ignore": "local"}], | |
| "scss/dollar-variable-pattern": ["^(root|_[a-z0-9\\-]+|ct-[a-z0-9\\-]+)$", {"ignore": "local"}], |
🤖 Prompt for AI Agents
In web/themes/custom/drevops/.stylelintrc.json around line 46, the
dollar-variable-pattern currently allows any characters for underscore-prefixed
and ct- variables; change the pattern to enforce kebab-case for both private
(underscore-prefixed) and ct- variables by replacing the ".+" parts with a
kebab-case character class: use a pattern that matches root, underscore followed
by lowercase letters/numbers/hyphens, or ct- followed by lowercase
letters/numbers/hyphens (e.g., "^(root|_[a-z0-9-]+|ct-[a-z0-9-]+)$"), keeping
the existing {"ignore": "local"} option.
| .ct-#{$prop-key}#{$dir-key}-#{$size-name} { | ||
| #{$prop-name}#{$dir1}: $size-value#{if($ct-spacing-use-important, ' !important', '')}; | ||
| #{$prop-name}#{$dir2}: $size-value#{if($ct-spacing-use-important, ' !important', '')}; | ||
| } | ||
| } | ||
| @else { | ||
| .ct-#{$prop-key}#{$dir-key}-#{$size-name} { | ||
| #{$prop-name}#{$dir-value}: $size-value#{if($ct-spacing-use-important, ' !important', '')}; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Micro: avoid repeating the important suffix computation.
Not critical, but you can compute once to reduce string interpolation noise.
Example:
- .ct-#{$prop-key}#{$dir-key}-#{$size-name} {
- #{$prop-name}#{$dir1}: $size-value#{if($ct-spacing-use-important, ' !important', '')};
- #{$prop-name}#{$dir2}: $size-value#{if($ct-spacing-use-important, ' !important', '')};
- }
+ $imp: if($ct-spacing-use-important, ' !important', '');
+ .ct-#{$prop-key}#{$dir-key}-#{$size-name} {
+ #{$prop-name}#{$dir1}: $size-value#{$imp};
+ #{$prop-name}#{$dir2}: $size-value#{$imp};
+ }Replicate similarly in the responsive block.
Also applies to: 83-90
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/00-base/spacing/spacing.scss around
lines 56 to 64 (and similarly 83 to 90), the string interpolation repeats the
#{if($ct-spacing-use-important, ' !important', '')} expression multiple times;
compute that suffix once into a local variable (e.g. $important-suffix:
if($ct-spacing-use-important, ' !important', '')) at the start of the block and
then append $important-suffix to each property value, and apply the same
refactor in the responsive block to remove duplicated interpolation noise.
| // CivicTheme variables can be found in | ||
| // <path/to/civictheme>/components/00-base/_variables.components.scss | ||
| // | ||
| // stylelint-disable scss/dollar-variable-pattern |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Re-enable the dollar-variable-pattern rule (or scope the disable).
With the updated pattern in stylelint, this global disable is likely unnecessary. Prefer removing it or scoping it narrowly if a specific line still needs it.
Apply:
-// stylelint-disable scss/dollar-variable-pattern
+// stylelint-enable scss/dollar-variable-patternOr remove the line entirely if lint passes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // stylelint-disable scss/dollar-variable-pattern | |
| // stylelint-enable scss/dollar-variable-pattern |
🤖 Prompt for AI Agents
In web/themes/custom/drevops/components/variables.components.scss at line 7,
there is a global "stylelint-disable scss/dollar-variable-pattern" which is too
broad; either remove this line entirely if the file now passes the updated
stylelint pattern, or scope the suppression to the exact offending lines by
replacing the global disable with targeted comments such as
"stylelint-disable-next-line scss/dollar-variable-pattern" immediately above the
specific variable declarations that fail, then re-run stylelint and adjust
variable names to match the pattern where possible.
Summary by CodeRabbit
New Features
Documentation
Style