fix(styles): default line heights#192
Conversation
📝 WalkthroughWalkthroughTypography CSS now uses custom properties for shared typography values, variants, and modifiers. Visual regression tests cover sizing and line-height overrides, while the Lighthouse compressed bundle-size threshold is increased. ChangesTypography custom-property refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. 🔧 ESLint
projects/styles/src/index.test.lighthouse.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. projects/styles/src/typography.cssESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. projects/styles/src/typography.test.visual.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the design-system typography defaults by adjusting theme line-height reference tokens and refactoring the nve-text utility CSS so text styles (size/weight/line-height/color/decoration) are driven by shared internal custom properties, with new/updated tests and baselines to validate the changes.
Changes:
- Adjusted
ref.font.lineHeighttoken values/descriptions in the themes token source and added a regression test asserting unitless relative line-height variables are emitted. - Refactored
projects/stylestypography utilities to use internal--_…variables for consistent application of font-size/weight/line-height/color/decoration, and added visual baselines for wrapped text line-height behavior. - Updated bundle-size guardrails to account for the typography CSS refactor and added new visual baseline assets.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/themes/src/index.test.ts | Adds a regression test verifying unitless relative line-height variables are generated. |
| projects/themes/src/index.json | Updates the default relative line-height token values and their descriptions. |
| projects/styles/src/typography.test.visual.ts | Adds visual regression coverage for wrapped text line-height behavior and an override scenario. |
| projects/styles/src/typography.css | Refactors nve-text typography utilities to use shared internal CSS variables and updates defaults. |
| projects/styles/src/index.test.lighthouse.ts | Adjusts the compressed bundle-size threshold for typography.css. |
| projects/styles/.visual/text-line-height.png | Adds a new visual regression baseline (LFS). |
| projects/styles/.visual/text-line-height-override.png | Adds a new visual regression baseline (LFS). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/typography.css`:
- Around line 160-176: Restrict the no-visit override in the typography link
state rules to the visited state only, so it cancels the visited color without
overriding hover feedback. Update the selector for &[nve-text~='no-visit'] near
the existing &:visited and &:hover rules, preserving the normal link color
behavior outside the visited state.
- Around line 88-109: Update the heading xl override in the
[nve-text~='heading'] typography rule to use the snug line-height token instead
of moderate, while preserving the existing font size and the xs moderate
override.
- Line 15: Insert a blank line before the first normal declaration following the
custom-property declarations in all six typography groups:
projects/styles/src/typography.css lines 15, 240, 270, 298, 320, and 348. Apply
this before font-family, color, font-weight, text-transform, line-height, and
text-align respectively; no declaration values or ordering need to change.
In `@projects/styles/src/typography.test.visual.ts`:
- Around line 42-61: Add a waitFor assertion in the test body around
visualRunner.render for the text-line-height-override scenario that selects one
body-text element, reads its computed line-height, and verifies it resolves to
the 1.5 override value before checking maxDiffPercentage. Keep the existing
visual regression assertion unchanged.
In `@projects/themes/src/index.test.ts`:
- Around line 45-53: Update the line-height assertions in the “should generate
unitless relative font line heights” test to include the semicolon terminating
each CSS custom-property declaration, ensuring values such as 1.2 do not match
longer values like 1.25.
🪄 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: 05941f8c-1c5e-4e84-a350-89d305d44639
⛔ Files ignored due to path filters (2)
projects/styles/.visual/text-line-height-override.pngis excluded by!**/*.pngprojects/styles/.visual/text-line-height.pngis excluded by!**/*.png
📒 Files selected for processing (5)
projects/styles/src/index.test.lighthouse.tsprojects/styles/src/typography.cssprojects/styles/src/typography.test.visual.tsprojects/themes/src/index.jsonprojects/themes/src/index.test.ts
3606787 to
cd8f21a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/typography.css`:
- Around line 12-20: Update the `link` and `list` typography blocks so
`--_font-size` and `--_font-weight` are both set to `inherit` instead of
resetting to initial or the regular reference weight, while preserving the
existing declarations for other text properties.
- Around line 220-223: Update the active-page selector a[aria-current='page'] in
the typography styles to preserve its semibold font weight with !important, or
set the selector’s --_font-weight value so it overrides the base link rule. Keep
the existing active-page color unchanged.
In `@projects/styles/src/typography.test.visual.ts`:
- Line 28: Update the layout container in the typography visual test to specify
an explicit height alongside its existing width, matching the constrained
dimensions used by the other active tests and the sibling text-size test setup.
In `@projects/themes/src/index.json`:
- Around line 1610-1613: Update the description for the loose lineHeights token
in the themes index so it describes the increased spacing outcome without using
the contradictory “dense” wording. Keep the guidance for captions, small print,
and body text, and ensure the revised wording is suitable for generated schema
and documentation.
🪄 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: 36d32b4d-20a7-46f7-a688-cd28c53c40a7
⛔ Files ignored due to path filters (2)
projects/styles/.visual/text-line-height-override.pngis excluded by!**/*.pngprojects/styles/.visual/text-line-height.pngis excluded by!**/*.png
📒 Files selected for processing (5)
projects/styles/src/index.test.lighthouse.tsprojects/styles/src/typography.cssprojects/styles/src/typography.test.visual.tsprojects/themes/src/index.jsonprojects/themes/src/index.test.ts
cd8f21a to
8a33957
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
projects/styles/src/typography.css:223
ul[nve-text~='nav'] li > a[aria-current='page']setsfont-weightwithout!important, but the base[nve-text~='link']rule appliesfont-weight: var(--_font-weight) !important;, so this semibold style will never win. Set the--_font-weightcustom property instead (fits the new variable-based refactor and works with the existing!importantfont-weight declaration).
> a[aria-current='page'] {
color: var(--nve-sys-text-link-hover-color) !important;
font-weight: var(--nve-ref-font-weight-semibold);
}
projects/styles/src/typography.css:154
- The refactor makes
[nve-text~='link']inherit the base typography variables, which currently default--_font-sizetoinitialand--_font-weighttoregular. Sincenve-text="link"is used widely without also specifyingbody/heading/..., this changes links from inheriting surrounding typography to forcing default size/weight. Consider explicitly setting link typography variables toinheritso inline links keep the surrounding text size/weight.
[nve-text~='link'] {
--_color: var(--nve-sys-text-link-color);
--_line-height: inherit;
--_text-decoration: underline;
8a33957 to
c1f6ea1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
projects/styles/src/typography.css:153
nve-text="link"is used in the codebase inside nav lists where the list items set the intendedfont-size(and sometimes weight) for the link text. Because the shared base rule appliesfont-size: var(--_font-size) !importantand defaults--_font-sizetoinitial, standalone links will no longer inherit their surrounding font size/weight.
Setting --_font-size/--_font-weight to inherit for the link utility restores the expected “link as modifier” behavior while keeping the variable-driven approach.
[nve-text~='link'] {
--_color: var(--nve-sys-text-link-color);
--_line-height: inherit;
--_text-decoration: underline;
projects/styles/src/typography.css:223
> a[aria-current='page']currently setsfont-weightwithout!important. This won’t override the!importantfont-weight coming from thenve-text="link"utility (which is how the nav list examples are authored), so the current-page link won’t actually render semibold.
> a:not(:hover) {
color: var(--nve-sys-text-muted-color) !important;
}
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/typography.css`:
- Around line 155-167: Update the link state selectors around typography.css
lines 155-167 to ensure disabled links retain disabled color over visited or
hover states, and update the color-modifier group around lines 232-247 so link
modifiers preserve link hover feedback. Apply the requested specificity or
selector scoping consistently at both sites, including link muted, emphasis,
white, black, and inherit variants.
🪄 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: 3b07567e-2aa6-4643-b15d-5d719ef79046
⛔ Files ignored due to path filters (2)
projects/styles/.visual/text-line-height-override.pngis excluded by!**/*.pngprojects/styles/.visual/text-line-height.pngis excluded by!**/*.png
📒 Files selected for processing (3)
projects/styles/src/index.test.lighthouse.tsprojects/styles/src/typography.cssprojects/styles/src/typography.test.visual.ts
| &[nve-text~='disabled'] { | ||
| --_color: var(--nve-sys-text-link-disabled-color); | ||
| } | ||
|
|
||
| &:visited, | ||
| &[nve-text~='visited'] { | ||
| --_color: var(--nve-sys-text-link-visited-color); | ||
| } | ||
|
|
||
| &:hover, | ||
| &[nve-text~='hover'] { | ||
| --_color: var(--nve-sys-text-link-hover-color); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Link state colors are decided by source order, not intent. Every state and modifier selector in the refactor lands at specificity (0,2,0), so whichever --_color declaration appears last in the file wins — which silently disables states the author expected to hold.
projects/styles/src/typography.css#L155-L167:&[nve-text~='disabled']precedes&:visited/&:hover, so disabled links still pick up hover and visited colors; exclude[nve-text~='disabled']from the state rules or give the disabled block higher specificity.projects/styles/src/typography.css#L232-L247: the color-modifier group sits after the wholelinkblock, sonve-text="link muted"(andemphasis/white/black/inherit) loses hover feedback; scope these with:not([nve-text~='link'])or re-add link-scoped color mappings inside thelinkblock.
📍 Affects 1 file
projects/styles/src/typography.css#L155-L167(this comment)projects/styles/src/typography.css#L232-L247
🤖 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/typography.css` around lines 155 - 167, Update the link
state selectors around typography.css lines 155-167 to ensure disabled links
retain disabled color over visited or hover states, and update the
color-modifier group around lines 232-247 so link modifiers preserve link hover
feedback. Apply the requested specificity or selector scoping consistently at
both sites, including link muted, emphasis, white, black, and inherit variants.
Signed-off-by: Cory Rylan <crylan@nvidia.com>
c1f6ea1 to
aa9c1e4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
projects/styles/src/typography.css:14
--_font-size: initial/--_line-height: initialin the base[nve-text~='…']rule forcesfont-size/line-heightto reset instead of inheriting from the surrounding text. This is a behavior regression for common patterns like<p nve-text="body …"><a nve-text="link">…</a></p>and<ul nve-text="list">…</ul>, where link/list are expected to inherit the parent’s typography unless explicitly sized.
--_color: var(--nve-sys-text-color);
--_font-weight: var(--nve-ref-font-weight-regular);
--_font-size: initial;
--_line-height: initial;
--_text-decoration: initial;
projects/styles/src/typography.css:154
- The refactor removed the link utility’s
xl/lg/smsizing behavior, andnve-text="link"no longer inherits font size/weight when used without also applying a base variant (e.g.body). There are multiple call sites that usenve-text="link"ornve-text="link sm"directly, so this likely changes typography unexpectedly.
[nve-text~='link'] {
--_color: var(--nve-sys-text-link-color);
--_font-size: inherit;
--_font-weight: inherit;
--_line-height: inherit;
There was a problem hiding this comment.
♻️ Duplicate comments (4)
projects/styles/src/typography.css (3)
15-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStylelint
declaration-empty-line-beforestill failing at 6 sites.Same rule as before — a normal declaration immediately follows the
--_*custom-property block in each group, which the repo's Stylelint config rejects. Sites (per current lint output): Line 15 (font-family, base group), Line 247 (color, color group), Line 277 (font-weight, weights group), Line 305 (text-transform, transforms group), Line 327 (line-height, line-heights group), Line 355 (text-align, text-align group).🛠️ Example fix (repeat pattern at each of the 6 sites)
--_text-decoration: initial; + font-family: var(--nve-ref-font-family) !important;Also applies to: 247-247, 277-277, 305-305, 327-327, 355-355
🤖 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/typography.css` at line 15, In the typography declaration groups, update the normal declarations immediately following each custom-property block—font-family, color, font-weight, text-transform, line-height, and text-align—to satisfy the repository’s declaration-empty-line-before Stylelint rule by adding the required empty line at all six sites.Source: Linters/SAST tools
88-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHeading
xlstill uses a looser line-height than every smaller heading.Ramp is non-monotonic:
xl(24px) =moderate(line 94),lg/base/sm/xsall resolve tosnug. The largest heading ends up the loosest, inverting the size/ratio relationship and contradictingmoderate's intended use (dense/interface text) vssnug(headings). This is the same concern raised previously; despite a prior "addressed" note, the moderate override forxlis still present in this code. It also gets baked into the newtext-line-heightvisual baseline (typography.test.visual.ts,heading xlrow), so shipping this locks the anomaly into the committed snapshot.🐛 Proposed fix if `xl` should stay snug
&[nve-text~='xl'] { --_font-size: var(--nve-ref-font-size-600); - --_line-height: var(--nve-ref-font-line-height-moderate); }🤖 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/typography.css` around lines 88 - 108, Update the xl override within the [nve-text~='heading'] typography rule to use the snug line-height token instead of moderate, while preserving its larger font-size token and the existing line-height behavior for all smaller heading sizes.
161-173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLink state colors are still decided by source order, not intent.
Every state/modifier selector here compiles to the same specificity, (0,2,0), so whichever
--_colordeclaration is last in source order wins — silently disabling states the author expects to hold:
- Lines 161-173:
&[nve-text~='disabled']precedes&:visited/&:hover; a disabled link that is also:hovered or:visitedpicks up the hover/visited color instead of staying disabled.- Lines 241-268: this color-modifier group sits after the whole
linkblock, sonve-text="link muted"(andemphasis/white/black/inherit) loses hover/visited feedback once hovered — the modifier's--_colorwins the tie at equal specificity purely by appearing later in the file.The
no-visitrule above (line 175) already demonstrates the fix pattern (bump specificity via:not()/extra state selectors) — apply the same idea here.🐛 Proposed fix sketch
- &[nve-text~='disabled'] { + &[nve-text~='disabled']:not(:hover):not(:visited) { --_color: var(--nve-sys-text-link-disabled-color); }-[nve-text~='emphasis'], -[nve-text~='muted'], -[nve-text~='white'], -[nve-text~='black'], -[nve-text~='inherit'] { +[nve-text~='emphasis']:not([nve-text~='link']), +[nve-text~='muted']:not([nve-text~='link']), +[nve-text~='white']:not([nve-text~='link']), +[nve-text~='black']:not([nve-text~='link']), +[nve-text~='inherit']:not([nve-text~='link']) {Also applies to: 241-268
🤖 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/typography.css` around lines 161 - 173, Update the link state selectors in the disabled/visited/hover group and the color-modifier group to use higher specificity, following the existing no-visit rule’s :not()/additional-state pattern. Ensure disabled remains dominant over visited and hover, while hover/visited feedback remains dominant over muted, emphasis, white, black, and inherit modifiers regardless of source order.projects/styles/src/typography.test.visual.ts (1)
42-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a computed-style assertion for the line-height override.
maxDiffPercentage < 1only guards the screenshot. Add a check on one body-text element's computedline-heightso a broken override can't be silently re-baselined into the failure state.🤖 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/typography.test.visual.ts` around lines 42 - 61, Add a computed-style assertion to the visual test around the text-line-height-override render, targeting one body-text paragraph and verifying its computed line-height is 1.5. Keep the existing maxDiffPercentage screenshot assertion and use the visual runner’s established DOM or browser evaluation API to inspect the rendered element.
🤖 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/typography.css`:
- Line 15: In the typography declaration groups, update the normal declarations
immediately following each custom-property block—font-family, color,
font-weight, text-transform, line-height, and text-align—to satisfy the
repository’s declaration-empty-line-before Stylelint rule by adding the required
empty line at all six sites.
- Around line 88-108: Update the xl override within the [nve-text~='heading']
typography rule to use the snug line-height token instead of moderate, while
preserving its larger font-size token and the existing line-height behavior for
all smaller heading sizes.
- Around line 161-173: Update the link state selectors in the
disabled/visited/hover group and the color-modifier group to use higher
specificity, following the existing no-visit rule’s :not()/additional-state
pattern. Ensure disabled remains dominant over visited and hover, while
hover/visited feedback remains dominant over muted, emphasis, white, black, and
inherit modifiers regardless of source order.
In `@projects/styles/src/typography.test.visual.ts`:
- Around line 42-61: Add a computed-style assertion to the visual test around
the text-line-height-override render, targeting one body-text paragraph and
verifying its computed line-height is 1.5. Keep the existing maxDiffPercentage
screenshot assertion and use the visual runner’s established DOM or browser
evaluation API to inspect the rendered element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6c4199c2-0654-42c4-bbeb-c91c5808077a
⛔ Files ignored due to path filters (2)
projects/styles/.visual/text-line-height-override.pngis excluded by!**/*.pngprojects/styles/.visual/text-line-height.pngis excluded by!**/*.png
📒 Files selected for processing (3)
projects/styles/src/index.test.lighthouse.tsprojects/styles/src/typography.cssprojects/styles/src/typography.test.visual.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
projects/styles/src/typography.css:245
- The generic color modifier block comes after the
[nve-text~='link']block, sonve-text="link emphasis"ends up using the generic--nve-sys-text-emphasis-colorinstead of the link-specific--nve-sys-text-link-emphasis-color(the later[nve-text~='emphasis']rule overwrites--_color). This makes the link emphasis variant ineffective.
[nve-text~='emphasis'],
[nve-text~='muted'],
[nve-text~='white'],
[nve-text~='black'],
[nve-text~='inherit'] {
Summary by CodeRabbit
Style
Tests
typography.css.