Skip to content

fix(styles): use logical direction styles#126

Open
coryrylan wants to merge 1 commit into
mainfrom
topic-cleanup-styles
Open

fix(styles): use logical direction styles#126
coryrylan wants to merge 1 commit into
mainfrom
topic-cleanup-styles

Conversation

@coryrylan

@coryrylan coryrylan commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator
  • 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.

Summary by CodeRabbit

  • Tests

    • Added test infrastructure and a new test to validate generated style metadata; introduced a project-specific test config.
  • Style

    • Refactored alignment, spacing, padding, and typography utilities to use logical CSS properties and updated capsize handling and view-transition linting.
  • Chores

    • Added project test and lint tasks to the build pipeline, introduced a stylelint config, and updated dev tooling for coverage.

@coryrylan coryrylan requested a review from johnyanarella June 9, 2026 00:53
@coryrylan coryrylan self-assigned this Jun 9, 2026
@github-actions github-actions Bot added scope(styles) dependencies Pull requests that update a dependency file labels Jun 9, 2026
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds 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.

Changes

Styles Testing, Linting, and CSS Modernization

Layer / File(s) Summary
Test infrastructure: Vitest config and metadata test
projects/styles/vitest.config.ts, projects/styles/src/metadata.test.ts, projects/styles/package.json
Vitest config with project root and path alias added; metadata test validates generated dist/data.html.json; package.json adds scripts.test, wireit.test, CI dependencies including test, build exclusion for src/**/*.test.ts, and a devDependency for @vitest/coverage-istanbul.
Linting infrastructure: Stylelint config and package.json wiring
projects/styles/stylelint.config.mjs, projects/styles/package.json, projects/styles/src/view-transitions.css
New stylelint.config.mjs composes base config and overrides rules; package lint wiring runs both lint:eslint and new lint:style; view-transitions.css adds stylelint-disable comments around an internal @keyframes name.
CSS modernization: Layout logical properties
projects/styles/src/layout.css
Layout alignment rules use place-items/place-content; padding utilities now set logical padding-inline/padding-block; spacing variants explicitly set gap to the space-none variable.
CSS modernization: Typography logical properties
projects/styles/src/typography.css
Capsize offset variable renamed and pseudo-element spacing uses margin-block-start/margin-block-end; list and nav spacing use padding-block/padding-inline and margin-block/margin-inline.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

scope(themes), scope(ci), type(fix)

Suggested reviewers

  • johnyanarella
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(styles): use logical direction styles' clearly summarizes the main change: refactoring CSS to use logical/combined properties instead of physical directional properties.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch topic-cleanup-styles

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.css

File contains syntax errors that prevent linting: Line 164: Unexpected value or character.; Line 164: expected ; but instead found ); Line 171: Unexpected value or character.; Line 171: expected ; but instead found ); Line 176: expected , but instead found $; Line 176: expected , but instead found logical-side; Line 176: expected , but instead found ); Line 176: expected , but instead found :; Line 176: expected , but instead found (; Line 176: expected , but instead found --nve-ref-space-; Line 176: expected , but instead found $; Line 176: expected , but instead found size; Line 176: expected , but instead found ); Line 180: expected , but instead found &; Line 181: Unexpected value or character.; Line 181: expected ; but instead found ); Line 185: Unexpected value or character.; Line 185: expected ; but instead found ); Line 200: Unexpected value or character.; Line 269: expected } but instead the file ends


Comment @coderabbitai help to get the list of available commands and usage tips.

- 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 coryrylan force-pushed the topic-cleanup-styles branch from 1ce3069 to 0b5affd Compare June 9, 2026 01:56

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1ce3069 and 0b5affd.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • projects/styles/package.json
  • projects/styles/src/layout.css
  • projects/styles/src/metadata.test.ts
  • projects/styles/src/typography.css
  • projects/styles/src/view-transitions.css
  • projects/styles/stylelint.config.mjs
  • projects/styles/vitest.config.ts

Comment thread projects/styles/src/layout.css
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file scope(ci) scope(styles)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant