fix(styles): use logical direction styles#126
Conversation
github-actions
Bot
added
scope(styles)
dependencies
📝 WalkthroughWalkthroughAdds Vitest and Stylelint infra to the styles package (configs, package wiring, a metadata test), updates CI/test wiring, and modernizes CSS to use logical properties for layout, spacing, padding, and typography. ChangesStyles Testing, Linting, and CSS Modernization
Sequence Diagram(s)sequenceDiagram
participant Wireit
participant Build
participant ViteInternals
participant Vitest
Wireit->>Build: depends on `build`
Wireit->>ViteInternals: depends on `../internals/vite:ci`
Wireit->>Vitest: run `vitest run` (src/** excluding lighthouse/visual)
Vitest->>Wireit: return test results / exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.4.16)projects/styles/src/layout.cssFile contains syntax errors that prevent linting: Line 174: Unexpected value or character.; Line 174: expected Comment |
coryrylan
force-pushed
the
topic-cleanup-styles
branch
from
1ce3069 to
0b5affd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/styles/src/layout.css`:
- Around line 174-177: The declaration-property interpolation
(padding-$(logical-side)) is the risky part; avoid interpolating into the
property name. Replace the interpolation by emitting explicit logical property
names inside the loop (e.g., set padding-block-start, padding-inline-start,
padding-inline-end, padding-block-end) for the corresponding $logical-side
values, keeping the same selector pattern &[nve-layout~='pad-$(side):$(size)']
and the same value var(--nve-ref-space-$(size)); use conditional mapping or
separate branches in the `@each` body that assign the correct explicit property
for each $logical-side instead of interpolating the property name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 452bedd1-a7cf-49b0-811a-bf0b9b98805f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
projects/styles/package.jsonprojects/styles/src/layout.cssprojects/styles/src/metadata.test.tsprojects/styles/src/typography.cssprojects/styles/src/view-transitions.cssprojects/styles/stylelint.config.mjsprojects/styles/vitest.config.ts
coryrylan
force-pushed
the
topic-cleanup-styles
branch
from
0b5affd to
a927e61
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
projects/styles/src/layout.css (1)
184-187:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winProperty name interpolation is not supported—emit explicit property names instead.
The syntax
padding-$(logical-side): ...attempts to interpolate a variable into the CSS property name itself, which PostCSS does not support. This causes the parse errors flagged by Biome and will likely break the build.🔧 Proposed fix
Replace the interpolated property name with explicit conditional logic:
- `@each` $side, $logical-side in (top, left, right, bottom), (block-start, inline-start, inline-end, block-end) { - &[nve-layout~='pad-$(side):$(size)'] { - padding-$(logical-side): var(--nve-ref-space-$(size)); - } - } + &[nve-layout~='pad-top:$(size)'] { + padding-block-start: var(--nve-ref-space-$(size)); + } + + &[nve-layout~='pad-left:$(size)'] { + padding-inline-start: var(--nve-ref-space-$(size)); + } + + &[nve-layout~='pad-right:$(size)'] { + padding-inline-end: var(--nve-ref-space-$(size)); + } + + &[nve-layout~='pad-bottom:$(size)'] { + padding-block-end: var(--nve-ref-space-$(size)); + }Alternatively, use a PostCSS mixin if property name interpolation is required:
`@define-mixin` pad-side $side, $logical-side { &[nve-layout~='pad-$(side):$(size)'] { padding-$(logical-side): var(--nve-ref-space-$(size)); } } `@mixin` pad-side top, block-start; `@mixin` pad-side left, inline-start; `@mixin` pad-side right, inline-end; `@mixin` pad-side bottom, block-end;However, verify that postcss-mixins supports property name interpolation before using this approach.
Verify the PostCSS toolchain's support for property name interpolation:
#!/bin/bash # Description: Check PostCSS config and test property name interpolation support echo "== Check PostCSS plugins configuration ==" cat projects/styles/postcss.config.cjs echo "" echo "== Test if postcss-mixins or postcss-each supports property-name interpolation ==" echo "Check documentation or try a minimal test case" echo "" echo "== Find other property-name interpolation patterns in codebase ==" rg -n "^[[:space:]]*[a-zA-Z-]+\-\\\$\\([^)]+\\)\\s*:" projects/styles/src --type=css || echo "No other property name interpolation found"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/styles/src/layout.css` around lines 184 - 187, The rule using property-name interpolation inside the `@each` (selector &[nve-layout~='pad-$(side):$(size)'] and the attempted property padding-$(logical-side)) must be replaced with explicit property names; update the `@each` or expand it into four explicit branches that map side->logical-side pairs (top->block-start => padding-block-start, left->inline-start => padding-inline-start, right->inline-end => padding-inline-end, bottom->block-end => padding-block-end) and use var(--nve-ref-space-$(size)) as the value, or alternatively replace the loop with a PostCSS mixin that emits those four explicit property names if your toolchain supports mixins.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/styles/src/layout.css`:
- Line 48: The CSS rule currently uses the shorthand property place-items:
flex-start which is misleading for a flex container; change this to use
align-items: flex-start and remove the place-items declaration so only the
effective flex property is present (search for the rule containing place-items
and replace that declaration with align-items: flex-start).
- Around line 142-143: The rule using place-items: center and place-content:
center under the non-grid selector [nve-layout~='align:center'] should be
changed to use explicit flex properties: replace place-items with align-items:
center and replace place-content with justify-content: center (add
align-content: center only if you need to affect multi-line cross-axis spacing);
keep the existing grid-specific rule ([nve-layout~='grid'] …
[nve-layout~='align:center']) untouched since it properly targets grid-only
behavior.
In `@projects/styles/src/metadata.test.ts`:
- Around line 44-49: The test helper expectAttributeValues currently asserts
each expected value individually (in the function expectAttributeValues), which
stops at the first missing value and yields poor diagnostics; change it to
gather all missing expected values from attributes.get(name) and assert once
that the missing list is empty (or that the set of expected values equals the
actual set) so a single assertion reports all missing items and provides clearer
failure output—locate the expectAttributeValues function and replace the
per-value expect loop with a single expect that compares expected values to
attributeValues (or asserts missingValues.length === 0) to show all differences
in one failure.
- Around line 44-49: Add a JSDoc block above the helper function
expectAttributeValues describing its purpose and parameters per the Google style
guide: include a one-line summary, a short description of behavior (it asserts
that a Map contains a Set for the given name and that the Set contains each
value), and `@param` tags for attributes (Map<string, Set<string>>), name
(string), and values (string[]), plus `@returns` {void}; mention that it may throw
assertion errors. Keep the comment concise and place it immediately above the
expectAttributeValues function declaration.
In `@projects/styles/stylelint.config.mjs`:
- Line 15: The config currently disables the stylelint rule named
custom-property-pattern; either remove the override so the root pattern (which
already permits names like --_capsize-offset and vscode-*) is enforced, or if
you truly need to disable it, add a one-line justification comment directly
above the custom-property-pattern entry explaining why enforcement must be
skipped (mention the specific exception like --_capsize-offset) so reviewers
know this is intentional. Ensure the change targets the custom-property-pattern
key in the stylelint configuration and that any retained disablement includes
the explanatory comment.
- Line 16: The rule 'media-query-no-invalid' is currently set to null in the
stylelint config which suppresses all invalid-media-query errors; either replace
the broad config-level disable with targeted inline suppressions by adding
stylelint-disable-next-line media-query-no-invalid immediately before the
specific `@media/`@container blocks that use the $(breakpoint-value) templating
(to avoid false positives elsewhere), or keep the config-level null but add a
short comment in the same config explaining why 'media-query-no-invalid' must be
disabled project-wide due to pervasive $(breakpoint-value) placeholders so
reviewers understand it is intentional.
---
Duplicate comments:
In `@projects/styles/src/layout.css`:
- Around line 184-187: The rule using property-name interpolation inside the
`@each` (selector &[nve-layout~='pad-$(side):$(size)'] and the attempted property
padding-$(logical-side)) must be replaced with explicit property names; update
the `@each` or expand it into four explicit branches that map side->logical-side
pairs (top->block-start => padding-block-start, left->inline-start =>
padding-inline-start, right->inline-end => padding-inline-end, bottom->block-end
=> padding-block-end) and use var(--nve-ref-space-$(size)) as the value, or
alternatively replace the loop with a PostCSS mixin that emits those four
explicit property names if your toolchain supports mixins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 440cc957-7900-42be-a325-55a2ed0bfd2d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
projects/styles/package.jsonprojects/styles/src/layout.cssprojects/styles/src/metadata.test.tsprojects/styles/src/typography.cssprojects/styles/src/view-transitions.cssprojects/styles/stylelint.config.mjsprojects/styles/vitest.config.ts
| flex-direction: row; | ||
| justify-items: flex-start; | ||
| align-items: flex-start; | ||
| place-items: flex-start; |
There was a problem hiding this comment.
Use align-items directly instead of place-items for flex containers.
place-items is a shorthand for align-items and justify-items, but justify-items has no effect on flex containers—only on grid. Using place-items: flex-start here is misleading because only the align-items portion takes effect.
✨ Proposed fix
- place-items: flex-start;
+ align-items: flex-start;📝 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.
| place-items: flex-start; | |
| align-items: flex-start; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/styles/src/layout.css` at line 48, The CSS rule currently uses the
shorthand property place-items: flex-start which is misleading for a flex
container; change this to use align-items: flex-start and remove the place-items
declaration so only the effective flex property is present (search for the rule
containing place-items and replace that declaration with align-items:
flex-start).
| place-items: center; | ||
| place-content: center; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find usage patterns of nve-layout~='align:center' to determine if it's used on flex or grid
echo "== Find nve-layout~='align:center' usage =="
rg -n -C3 "nve-layout.*align:center" --type=html --type=ts --type=tsx --type=jsx || echo "No HTML/TS usage found"
echo ""
echo "== Check if align:center is used with row/column/grid =="
rg -n "nve-layout.*(?=.*align:center)(?=.*(?:row|column|grid))" --type=html --type=ts --type=tsx --type=jsx || echo "No combined usage found"Repository: NVIDIA/elements
Length of output: 525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect layout.css around lines 142-143 =="
nl -ba projects/styles/src/layout.css | sed -n '120,190p'
echo ""
echo "== Find occurrences of align:center selector in layout.css =="
rg -n "\[nve-layout~='align:center'\]" projects/styles/src/layout.css
echo ""
echo "== Show surrounding rules for grid align center =="
rg -n "\[nve-layout~='grid'\]" projects/styles/src/layout.css
echo ""
echo "== Find place-items/place-content in layout.css =="
rg -n "place-items:\s*center|place-content:\s*center" projects/styles/src/layout.cssRepository: NVIDIA/elements
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect layout.css around lines 120-190 (for context around 142-143) =="
cat -n projects/styles/src/layout.css | sed -n '120,190p'
echo ""
echo "== Find occurrences of [nve-layout~='align:center'] in layout.css =="
rg -n "\\[nve-layout~='align:center'\\]" projects/styles/src/layout.css
echo ""
echo "== Show surrounding rules for grid align:center within layout.css =="
rg -n -C3 "\\[nve-layout~='grid'\\]" projects/styles/src/layout.css | head -n 200
echo ""
echo "== Find all place-items/place-content center rules in layout.css =="
rg -n "place-items:\s*center|place-content:\s*center" projects/styles/src/layout.cssRepository: NVIDIA/elements
Length of output: 2905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect layout.css around flex/grid and nested align:center =="
cat -n projects/styles/src/layout.css | sed -n '180,230p'
echo ""
cat -n projects/styles/src/layout.css | sed -n '230,310p'Repository: NVIDIA/elements
Length of output: 3337
Clarify flex centering rule: place-items/place-content works, but the shorthand mixes grid-only parts
In projects/styles/src/layout.css, the non-grid [nve-layout~='align:center'] rule (lines 142-144) sets place-items: center and place-content: center; in flexbox this still yields the intended align-items: center and justify-content: center (the justify-items portion of place-items is grid-only and won’t be used). Grid containers already override with a dedicated [nve-layout~='grid'] … [nve-layout~='align:center'] rule (lines 266-270).
Consider using explicit align-items: center + justify-content: center (and align-content only if needed) to avoid the grid-only justify-items component in place-items.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/styles/src/layout.css` around lines 142 - 143, The rule using
place-items: center and place-content: center under the non-grid selector
[nve-layout~='align:center'] should be changed to use explicit flex properties:
replace place-items with align-items: center and replace place-content with
justify-content: center (add align-content: center only if you need to affect
multi-line cross-axis spacing); keep the existing grid-specific rule
([nve-layout~='grid'] … [nve-layout~='align:center']) untouched since it
properly targets grid-only behavior.
| function expectAttributeValues(attributes: Map<string, Set<string>>, name: string, values: string[]) { | ||
| const attributeValues = attributes.get(name); | ||
|
|
||
| expect(attributeValues).toBeDefined(); | ||
| values.forEach(value => expect(attributeValues?.has(value)).toBe(true)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Improve test failure diagnostics.
The current implementation stops at the first missing value, making it harder to diagnose when multiple values are missing. Consider asserting all values at once for clearer failure output.
♻️ Proposed refactor for better error messages
function expectAttributeValues(attributes: Map<string, Set<string>>, name: string, values: string[]) {
const attributeValues = attributes.get(name);
expect(attributeValues).toBeDefined();
- values.forEach(value => expect(attributeValues?.has(value)).toBe(true));
+ const missingValues = values.filter(value => !attributeValues?.has(value));
+ expect(missingValues).toEqual([]);
}This will show all missing values in a single assertion failure.
📝 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.
| function expectAttributeValues(attributes: Map<string, Set<string>>, name: string, values: string[]) { | |
| const attributeValues = attributes.get(name); | |
| expect(attributeValues).toBeDefined(); | |
| values.forEach(value => expect(attributeValues?.has(value)).toBe(true)); | |
| } | |
| function expectAttributeValues(attributes: Map<string, Set<string>>, name: string, values: string[]) { | |
| const attributeValues = attributes.get(name); | |
| expect(attributeValues).toBeDefined(); | |
| const missingValues = values.filter(value => !attributeValues?.has(value)); | |
| expect(missingValues).toEqual([]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/styles/src/metadata.test.ts` around lines 44 - 49, The test helper
expectAttributeValues currently asserts each expected value individually (in the
function expectAttributeValues), which stops at the first missing value and
yields poor diagnostics; change it to gather all missing expected values from
attributes.get(name) and assert once that the missing list is empty (or that the
set of expected values equals the actual set) so a single assertion reports all
missing items and provides clearer failure output—locate the
expectAttributeValues function and replace the per-value expect loop with a
single expect that compares expected values to attributeValues (or asserts
missingValues.length === 0) to show all differences in one failure.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Add JSDoc for the helper function.
Per coding guidelines, TypeScript functions should follow the Google developer documentation style guide with JSDoc comments.
📝 Proposed JSDoc
+/**
+ * Asserts that an attribute exists and contains all expected values.
+ * `@param` attributes - Map of attribute names to their value sets
+ * `@param` name - Attribute name to check
+ * `@param` values - Expected values that should exist for the attribute
+ */
function expectAttributeValues(attributes: Map<string, Set<string>>, name: string, values: string[]) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/styles/src/metadata.test.ts` around lines 44 - 49, Add a JSDoc block
above the helper function expectAttributeValues describing its purpose and
parameters per the Google style guide: include a one-line summary, a short
description of behavior (it asserts that a Map contains a Set for the given name
and that the Set contains each value), and `@param` tags for attributes
(Map<string, Set<string>>), name (string), and values (string[]), plus `@returns`
{void}; mention that it may throw assertion errors. Keep the comment concise and
place it immediately above the expectAttributeValues function declaration.
Source: Coding guidelines
| } | ||
| ], | ||
| 'at-rule-empty-line-before': null, | ||
| 'custom-property-pattern': null, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find custom properties that may not follow the root pattern
# Search for custom property definitions in styles
rg -nP --type=css '^\s*--[a-zA-Z0-9_-]+:' projects/styles/src/ | rg -v '^\s*--(_?[a-z][a-z0-9]*(-[a-z0-9]+)*|vscode-.+):'Repository: NVIDIA/elements
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show surrounding context of the local disable
echo "== Local projects/styles/stylelint.config.mjs around line 15 =="
nl -ba projects/styles/stylelint.config.mjs | sed -n '1,60p'
# 2) Find all occurrences of custom-property-pattern in repo
echo
echo "== Occurrences of custom-property-pattern =="
rg -n "custom-property-pattern" -S .
# 3) Show candidate root/extended config(s) that set the pattern
# (Print the surrounding lines for each match)
echo
echo "== Context around each custom-property-pattern usage =="
rg -n "custom-property-pattern" -S . -C 3
# 4) Search for any other custom properties with underscore/odd naming in styles
echo
echo "== Custom properties in projects/styles/src that start with --_ =="
rg -nP --type=css '^\s*--_([A-Za-z0-9_-]+)\s*:' projects/styles/src/Repository: NVIDIA/elements
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Local projects/styles/stylelint.config.mjs around line 15 =="
# Use awk to print line numbers since nl may be unavailable
awk 'NR>=8 && NR<=25 { printf "%d:%s\n", NR, $0 }' projects/styles/stylelint.config.mjs
echo
echo "== Occurrences of custom-property-pattern =="
rg -n "custom-property-pattern" -S .
echo
echo "== Context around each custom-property-pattern usage (3 lines before/after) =="
rg -n "custom-property-pattern" -S . -C 3
echo
echo "== Custom properties in projects/styles/src that start with --_ =="
rg -nP --type=css '^\s*--_([A-Za-z0-9_-]+)\s*:' projects/styles/src/Repository: NVIDIA/elements
Length of output: 2135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Line count =="
wc -l stylelint.config.mjs projects/styles/stylelint.config.mjs
echo
echo "== Root ./stylelint.config.mjs =="
sed -n '1,120p' stylelint.config.mjs
echo
echo "== projects/styles/stylelint.config.mjs =="
sed -n '1,120p' projects/styles/stylelint.config.mjsRepository: NVIDIA/elements
Length of output: 2737
🌐 Web query:
stylelint custom-property-pattern matches --name including leading double hyphen or without
💡 Result:
In Stylelint, the custom-property-pattern rule evaluates custom property names by ignoring the leading double hyphen (--) [1]. Specifically, the rule's internal logic slices the first two characters (the --) from the property name before testing it against the regex pattern provided [1]. As a result, your configured regex pattern should not include the leading double hyphen, as Stylelint will have already removed it during validation [1]. For example, if you want to enforce kebab-case for custom properties (e.g., --my-property), you should define the pattern as ^[a-z0-9-]+$ rather than ^--[a-z0-9-]+$ [2][3]. If your regex pattern explicitly requires a double hyphen as part of the match, it will fail because the rule effectively only checks the portion of the string following the -- prefix [1]. If you need to enforce a naming convention that includes specific double-hyphenated prefixes, you should design your regex to account only for the characters that appear after the mandatory leading -- [1][4].
Citations:
- 1: https://github.com/stylelint/stylelint/blob/main/lib/rules/custom-property-pattern/index.mjs
- 2: https://stylelint.io/user-guide/rules/custom-property-pattern
- 3: https://github.com/stylelint/stylelint/blob/master/lib/rules/custom-property-pattern/README.md
- 4: https://stackoverflow.com/questions/76501969/need-a-custom-stylelint-rule-or-plugin-that-keeps-kebab-case-but-allows-wp
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract all custom property names (without the leading --) and test against root regex
python3 - <<'PY'
import re, subprocess, shlex
root_re = re.compile(r'^(_?[a-z][a-z0-9]*(-[a-z0-9]+)*|vscode-.+)$')
cmd = r"rg -nP --type=css r'^\s*--[a-zA-Z0-9_-]+:' projects/styles/src/"
out = subprocess.check_output(cmd, shell=True, text=True)
bad = []
for line in out.splitlines():
# line format: path:lineno:content
path, lineno, content = line.split(":", 2)
m = re.match(r'^\s*--([a-zA-Z0-9_-]+):', content)
if not m:
continue
name = m.group(1)
if not root_re.match(name):
bad.append((path, lineno, "--"+name))
if not bad:
print("No custom properties in projects/styles/src would fail the root custom-property-pattern.")
else:
print("Custom properties failing root custom-property-pattern:")
for path, lineno, full in bad:
print(f"{path}:{lineno}:{full}")
PYRepository: NVIDIA/elements
Length of output: 591
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re, subprocess
root_re = re.compile(r'^(_?[a-z][a-z0-9]*(-[a-z0-9]+)*|vscode-.+)$')
res = subprocess.run(
["rg","-nP","--type=css",r"^\s*--[a-zA-Z0-9_-]+:", "projects/styles/src/"],
text=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
out = res.stdout.strip()
bad = []
if out:
for line in out.splitlines():
path, lineno, content = line.split(":", 2)
m = re.match(r'^\s*--([a-zA-Z0-9_-]+):', content)
if not m:
continue
name = m.group(1)
if not root_re.match(name):
bad.append((path, lineno, "--" + name))
if not bad:
print("No custom properties in projects/styles/src would fail the root custom-property-pattern.")
else:
print("Custom properties failing root custom-property-pattern:")
for path, lineno, full in bad:
print(f"{path}:{lineno}:{full}")
PYRepository: NVIDIA/elements
Length of output: 148
Confirm whether disabling custom-property-pattern is necessary (projects/styles/stylelint.config.mjs:15)
- The root
custom-property-pattern(^(_?[a-z][a-z0-9]*(-[a-z0-9]+)*|vscode-.+)$) matches the existing custom properties inprojects/styles/src, including--_capsize-offset. - Remove the override (
custom-property-pattern: null) to keep enforcement, or add a justification comment for why this rule must be disabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/styles/stylelint.config.mjs` at line 15, The config currently
disables the stylelint rule named custom-property-pattern; either remove the
override so the root pattern (which already permits names like --_capsize-offset
and vscode-*) is enforced, or if you truly need to disable it, add a one-line
justification comment directly above the custom-property-pattern entry
explaining why enforcement must be skipped (mention the specific exception like
--_capsize-offset) so reviewers know this is intentional. Ensure the change
targets the custom-property-pattern key in the stylelint configuration and that
any retained disablement includes the explanatory comment.
- Use logical direction styles - Introduced a new test command in package.json to run tests using vitest. - Added a stylelint configuration file for CSS linting with custom rules. - Created a new test file for metadata validation to ensure correct attribute generation. Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cleanup-styles
branch
from
a927e61 to
60a0499
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
projects/styles/src/layout.css (3)
48-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
align-itemsdirectly instead ofplace-itemsfor flex containers.
place-itemsis a shorthand foralign-itemsandjustify-items, butjustify-itemshas no effect on flex containers—only on grid. Usingplace-items: flex-starthere is misleading because only thealign-itemsportion takes effect.✨ Proposed fix
- place-items: flex-start; + align-items: flex-start;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/styles/src/layout.css` at line 48, Replace the misleading use of the CSS shorthand place-items in the layout.css flex container with the explicit property align-items: locate where "place-items: flex-start;" is applied (the flex container rule) and change it to use align-items: flex-start so the intention is clear and no grid-only property is referenced.
142-143: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify flex centering rule: use explicit properties instead of shorthand.
In flexbox,
place-items: centersets bothalign-items: center(effective) andjustify-items: center(ignored in flex). For clarity, use explicitalign-items: centerandjustify-content: centerinstead of the grid-oriented shorthands.♻️ Proposed refactor
- place-items: center; - place-content: center; + align-items: center; + justify-content: center;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/styles/src/layout.css` around lines 142 - 143, The current CSS uses grid shorthands place-items and place-content which are misleading for a flex container; replace place-items: center with explicit align-items: center and replace place-content: center with justify-content: center (or both align-content/justify-content as appropriate for multi-line flex) in the relevant rules so the intent is clear and works correctly for flex layouts; locate usages of place-items and place-content in the stylesheet and update them to align-items/justify-content (and align-content if needed) accordingly.
184-187:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Property name interpolation is not supported by PostCSS.
Line 186 attempts to interpolate into the CSS property name itself:
padding-$(logical-side). This syntax is not supported by postcss-each and causes parse errors. The multi-variable@eachdestructuring works, but you cannot interpolate variable values into property names.🐛 Proposed fix
Replace the interpolated property name with explicit property assignments:
- `@each` $side, $logical-side in (top, left, right, bottom), (block-start, inline-start, inline-end, block-end) { + &[nve-layout~='pad-top:$(size)'] { + padding-block-start: var(--nve-ref-space-$(size)); + } + + &[nve-layout~='pad-left:$(size)'] { + padding-inline-start: var(--nve-ref-space-$(size)); + } + + &[nve-layout~='pad-right:$(size)'] { + padding-inline-end: var(--nve-ref-space-$(size)); + } + + &[nve-layout~='pad-bottom:$(size)'] { + padding-block-end: var(--nve-ref-space-$(size)); + } - &[nve-layout~='pad-$(side):$(size)'] { - padding-$(logical-side): var(--nve-ref-space-$(size)); - } - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/styles/src/layout.css` around lines 184 - 187, The issue is that property-name interpolation `padding-$(logical-side)` inside the `@each` loop is unsupported by PostCSS and causes parse errors; update the loop that uses `@each $side, $logical-side in (top, left, right, bottom), (block-start, inline-start, inline-end, block-end)` and the selector `&[nve-layout~='pad-$(side):$(size)']` to assign concrete properties instead of interpolating the name—for example, map each `$logical-side` to its explicit CSS property and output `padding-top`, `padding-left`, `padding-right`, `padding-bottom` (and the corresponding logical properties) in separate branches or use an explicit mapping/mixin so the generated property names are literal rather than interpolated.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@projects/styles/src/layout.css`:
- Line 48: Replace the misleading use of the CSS shorthand place-items in the
layout.css flex container with the explicit property align-items: locate where
"place-items: flex-start;" is applied (the flex container rule) and change it to
use align-items: flex-start so the intention is clear and no grid-only property
is referenced.
- Around line 142-143: The current CSS uses grid shorthands place-items and
place-content which are misleading for a flex container; replace place-items:
center with explicit align-items: center and replace place-content: center with
justify-content: center (or both align-content/justify-content as appropriate
for multi-line flex) in the relevant rules so the intent is clear and works
correctly for flex layouts; locate usages of place-items and place-content in
the stylesheet and update them to align-items/justify-content (and align-content
if needed) accordingly.
- Around line 184-187: The issue is that property-name interpolation
`padding-$(logical-side)` inside the `@each` loop is unsupported by PostCSS and
causes parse errors; update the loop that uses `@each $side, $logical-side in
(top, left, right, bottom), (block-start, inline-start, inline-end, block-end)`
and the selector `&[nve-layout~='pad-$(side):$(size)']` to assign concrete
properties instead of interpolating the name—for example, map each
`$logical-side` to its explicit CSS property and output `padding-top`,
`padding-left`, `padding-right`, `padding-bottom` (and the corresponding logical
properties) in separate branches or use an explicit mapping/mixin so the
generated property names are literal rather than interpolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 666ed3ad-dd61-40ac-9f1a-914a873cbb61
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
projects/styles/package.jsonprojects/styles/src/layout.cssprojects/styles/src/metadata.test.tsprojects/styles/src/typography.cssprojects/styles/src/view-transitions.cssprojects/styles/stylelint.config.mjsprojects/styles/vitest.config.tsprojects/styles/vitest.lighthouse.ts
2 participants
Summary by CodeRabbit
Tests
Style
Chores