feat(typography): BREAKING CHANGE support inter font#4676
Conversation
tjuanitas
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds InterVariable font assets and body-based font switching, updates shared typography mixins and constants, and migrates component styles to the new base, legacy, and minimum font-size tokens. ChangesInter Font Rollout
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
src/styles/mixins/_typography.scss (1)
34-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a variable instead of hardcoded
14px/450.Unlike
bdl-fonts--dejaBlue, which sources its size from$bdl-fontSize--dejaBlue, this mixin hardcodesfont-size: 14pxandfont-weight: 450. Extracting these into named variables (e.g.$bdl-fontSize--inter,$bdl-fontWeight--inter) would keep the pattern consistent and easier to tune later.🤖 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 `@src/styles/mixins/_typography.scss` around lines 34 - 40, The bdl-fonts--inter mixin hardcodes the font size and weight values, so update it to use named typography variables instead. Add or reuse variables such as $bdl-fontSize--inter and $bdl-fontWeight--inter, then reference them inside bdl-fonts--inter alongside the existing bdl-fontFamily--inter and font-aliasing includes to keep it consistent with bdl-fonts--dejaBlue.
🤖 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 `@src/styles/mixins/_typography.scss`:
- Around line 8-11: The mixin names in the typography styles use camelCase
segments and fail the scss/at-mixin-pattern kebab-case rule. Rename the affected
mixins in the typography mixin file to fully kebab-case, then update every call
site that references them, including the common variables, common typography
styles, and the dropdown menu stylesheet, so all usages match the new names
consistently.
---
Nitpick comments:
In `@src/styles/mixins/_typography.scss`:
- Around line 34-40: The bdl-fonts--inter mixin hardcodes the font size and
weight values, so update it to use named typography variables instead. Add or
reuse variables such as $bdl-fontSize--inter and $bdl-fontWeight--inter, then
reference them inside bdl-fonts--inter alongside the existing
bdl-fontFamily--inter and font-aliasing includes to keep it consistent with
bdl-fonts--dejaBlue.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43f927da-3d30-4be8-9ac2-08906f6254e9
📒 Files selected for processing (6)
.storybook/preview-head.htmlsrc/components/dropdown-menu/DropdownMenu.scsssrc/elements/common/_variables.scsssrc/elements/common/fonts.scsssrc/styles/common/_typography.scsssrc/styles/mixins/_typography.scss
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@src/components/label-pill/LabelPill.scss`:
- Line 17: The LabelPill stylesheet is using an invalid CSS custom property
reference for the large pill font size. Update the font-size declaration in
LabelPill.scss to use the proper var(--legacy) syntax so the intended 13px size
is applied instead of being ignored.
In `@src/features/content-insights/_mixins.scss`:
- Line 6: The font-size declaration in the content-insights mixin is using a
bare custom property reference, which makes the rule invalid. Update the
declaration in the mixin that defines the `font-size` value to use
`var(--minimum)` so the stylesheet resolves the custom property correctly.
In `@src/features/sandbox-banner/SandboxBanner.scss`:
- Line 27: The font-size declaration in SandboxBanner.scss is using the custom
property name directly, which is invalid CSS and causes
`.bdl-SandboxBanner-text` to inherit the font size instead. Update the rule in
the SandboxBanner stylesheet to reference the CSS variable properly via `var()`
so the `font-size` applies as intended.
In `@src/styles/common/_buttons.scss`:
- Line 20: The shared button base class in the `.btn` styles is using an invalid
font-size value, so update the base button styling to use the proper CSS custom
property form with `var()` instead of a bare token. Locate the `.btn` rule in
the button stylesheet and correct the `font-size` declaration so the shared base
sizing applies consistently across all buttons.
In `@src/styles/constants/_typography.scss`:
- Around line 4-7: The typography constants file contains invalid Sass variable
declarations, so fix the missing $bdl-fontSize prefix on the font-size values in
_typography.scss. Update the related consumers in the _typography mixin so any
font-size: --legacy style references use the matching $bdl-fontSize* variables
instead. Use the _typography.scss constants and the _typography mixin as the key
places to verify the variable names are consistent and parse correctly.
In `@src/styles/mixins/_typography.scss`:
- Around line 7-9: The typography mixin selector currently breaks the body use
case because `@at-root :global(:where(body.is-inter-font)) &` compiles to a dead
`body.is-inter-font body` match when included on `body`. Update the mixin in
`_typography.scss` to keep the descendant selector for component call sites like
`Tooltip` and `.flyout`, but add a separate special-case path for `body` in the
typography mixin logic so the Inter font override applies correctly there.
Reference the existing mixin block that emits the `font-family` rule and
preserve behavior for non-body selectors.
- Line 23: The typography mixins currently use an invalid placeholder value for
font size, and both legacy declarations need to be restored to use the proper
Sass variable. Update the font-size handling in the mixins in _typography.scss
so the legacy branch uses bdl-fontSize--legacy, and make sure the related
constants in _typography.scss are valid so the variable resolves correctly. Use
the existing mixin names and the bdl-fontSize variable family to locate and fix
both declarations consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fa19e67-35cc-43a3-90c4-aea99224bde8
📒 Files selected for processing (22)
src/components/category-selector/CategorySelector.scsssrc/components/dropdown-menu/DropdownMenu.scsssrc/components/form-elements/draft-js-mention-selector/DraftTimestamp.scsssrc/components/label-pill/LabelPill.scsssrc/components/pill-selector-dropdown/RoundPill.scsssrc/components/tooltip/Tooltip.scsssrc/elements/content-sidebar/activity-feed/comment/BaseComment.scsssrc/elements/content-sidebar/activity-feed/comment/components/BaseCommentInfo.scsssrc/elements/content-sidebar/activity-feed/common/activity-message/ActivityMessage.scsssrc/features/collapsible-sidebar/CollapsibleSidebar.scsssrc/features/content-explorer/content-explorer/ContentExplorer.scsssrc/features/content-insights/ContentAnalyticsErrorState.scsssrc/features/content-insights/_mixins.scsssrc/features/sandbox-banner/SandboxBanner.scsssrc/styles/common/_buttons.scsssrc/styles/common/_typography.scsssrc/styles/constants/_typography.scsssrc/styles/mixins/_buttons.scsssrc/styles/mixins/_typography.scsssrc/styles/variables.jssrc/styles/variables.jsonsrc/styles/variables.ts
✅ Files skipped from review due to trivial changes (5)
- src/features/content-insights/ContentAnalyticsErrorState.scss
- src/features/collapsible-sidebar/CollapsibleSidebar.scss
- src/components/pill-selector-dropdown/RoundPill.scss
- src/elements/content-sidebar/activity-feed/comment/components/BaseCommentInfo.scss
- src/components/dropdown-menu/DropdownMenu.scss
tjuanitas
force-pushed
the
feat-add-inter-font
branch
from
b6cdf3a to
978a6bd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/styles/mixins/_typography.scss (2)
7-9: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftBody-selector bug still unresolved:
body.is-inter-font bodynever matches.
@at-root :global(:where(body.is-inter-font)) &compiles tobody.is-inter-font body(orbody.is-inter-font body.is-inter-fontinbdl-fonts/bdl-commonTypography) when these mixins are included directly on abodyselector, so the Inter overrides silently never apply there. This is the same issue flagged in a prior review round on lines 7-9 and has not been addressed — it's now duplicated across all three@at-rootblocks (font-family, bdl-fonts, bdl-commonTypography), compounding the impact.Special-case
bodyso the override applies to the element itself rather than a descendant:🐛 Proposed fix
`@mixin` font-family { font-family: Lato, 'Helvetica Neue', Helvetica, Arial, sans-serif; - `@at-root` :global(:where(body.is-inter-font)) & { - font-family: InterVariable, 'Helvetica Neue', Helvetica, Arial, sans-serif; - } + `@at-root` :global(:where(body.is-inter-font)) &, + :global(&:where(body.is-inter-font)) { + font-family: InterVariable, 'Helvetica Neue', Helvetica, Arial, sans-serif; + } }Apply the analogous fix to the blocks in
bdl-fonts(33-35) andbdl-commonTypography(53-55).Also applies to: 33-35, 53-55
🤖 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 `@src/styles/mixins/_typography.scss` around lines 7 - 9, The Inter override selector in the typography mixins is still generating a descendant body selector that never matches when the mixins are applied directly to body. Update the `@at-root :global(:where(body.is-inter-font)) &` blocks in `font-family`, `bdl-fonts`, and `bdl-commonTypography` to special-case `body` so the override targets the element itself instead of `body` as a child selector, while preserving the existing behavior for non-body selectors.
46-46: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
bdl-commonTypographystill violates kebab-case mixin naming (scss/at-mixin-pattern).Static analysis flags this at line 46 (camelCase
Typographysegment). This will fail CI stylelint checks. A prior review flagged the same category of naming issue on the older mixin variants; the mixin has since been renamed but still doesn't comply. Renaming requires updating all call sites (e.g.src/elements/common/_variables.scss,src/styles/common/_typography.scss,src/components/dropdown-menu/DropdownMenu.scssper the stack context).🎨 Suggested kebab-case naming
-@mixin bdl-commonTypography { +@mixin bdl-common-typography { `@include` bdl-fonts;🤖 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 `@src/styles/mixins/_typography.scss` at line 46, The mixin name still violates the kebab-case stylelint rule, so rename bdl-commonTypography to a kebab-case mixin name in _typography.scss and update every include/use site that references it. Make sure the related call sites in _variables.scss, common/_typography.scss, and DropdownMenu.scss are changed consistently so the new mixin name compiles and passes scss/at-mixin-pattern.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 `@src/styles/mixins/_typography.scss`:
- Around line 7-9: The Inter override selector in the typography mixins is still
generating a descendant body selector that never matches when the mixins are
applied directly to body. Update the `@at-root
:global(:where(body.is-inter-font)) &` blocks in `font-family`, `bdl-fonts`, and
`bdl-commonTypography` to special-case `body` so the override targets the
element itself instead of `body` as a child selector, while preserving the
existing behavior for non-body selectors.
- Line 46: The mixin name still violates the kebab-case stylelint rule, so
rename bdl-commonTypography to a kebab-case mixin name in _typography.scss and
update every include/use site that references it. Make sure the related call
sites in _variables.scss, common/_typography.scss, and DropdownMenu.scss are
changed consistently so the new mixin name compiles and passes
scss/at-mixin-pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b57b1397-9d3a-420b-8f69-28db62c0d9a3
📒 Files selected for processing (22)
src/components/category-selector/CategorySelector.scsssrc/components/dropdown-menu/DropdownMenu.scsssrc/components/form-elements/draft-js-mention-selector/DraftTimestamp.scsssrc/components/label-pill/LabelPill.scsssrc/components/pill-selector-dropdown/RoundPill.scsssrc/components/tooltip/Tooltip.scsssrc/elements/content-sidebar/activity-feed/comment/BaseComment.scsssrc/elements/content-sidebar/activity-feed/comment/components/BaseCommentInfo.scsssrc/elements/content-sidebar/activity-feed/common/activity-message/ActivityMessage.scsssrc/features/collapsible-sidebar/CollapsibleSidebar.scsssrc/features/content-explorer/content-explorer/ContentExplorer.scsssrc/features/content-insights/ContentAnalyticsErrorState.scsssrc/features/content-insights/_mixins.scsssrc/features/sandbox-banner/SandboxBanner.scsssrc/styles/common/_buttons.scsssrc/styles/common/_typography.scsssrc/styles/constants/_typography.scsssrc/styles/mixins/_buttons.scsssrc/styles/mixins/_typography.scsssrc/styles/variables.jssrc/styles/variables.jsonsrc/styles/variables.ts
✅ Files skipped from review due to trivial changes (10)
- src/features/collapsible-sidebar/CollapsibleSidebar.scss
- src/components/dropdown-menu/DropdownMenu.scss
- src/components/form-elements/draft-js-mention-selector/DraftTimestamp.scss
- src/components/pill-selector-dropdown/RoundPill.scss
- src/features/content-insights/ContentAnalyticsErrorState.scss
- src/styles/mixins/_buttons.scss
- src/features/content-insights/_mixins.scss
- src/elements/content-sidebar/activity-feed/common/activity-message/ActivityMessage.scss
- src/elements/content-sidebar/activity-feed/comment/components/BaseCommentInfo.scss
- src/features/sandbox-banner/SandboxBanner.scss
🚧 Files skipped from review as they are similar to previous changes (10)
- src/features/content-explorer/content-explorer/ContentExplorer.scss
- src/components/tooltip/Tooltip.scss
- src/elements/content-sidebar/activity-feed/comment/BaseComment.scss
- src/components/category-selector/CategorySelector.scss
- src/styles/common/_buttons.scss
- src/styles/constants/_typography.scss
- src/styles/variables.json
- src/styles/common/_typography.scss
- src/styles/variables.ts
- src/styles/variables.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/common/_variables.scss (1)
3-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the remaining
$fontreference.src/elements/content-open-with/ContentOpenWith.scss:4still uses$font, so removing it from the shared variables will break the stylesheet build.🤖 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 `@src/elements/common/_variables.scss` at line 3, The shared SCSS variables still have a remaining $font dependency in ContentOpenWith.scss, so update the stylesheet that still references it to use the new font variable or a local replacement before removing $font from _variables.scss. Locate the usage in ContentOpenWith and any related shared mixins/variables, then ensure the build no longer relies on $font anywhere.
♻️ Duplicate comments (2)
src/styles/mixins/_typography.scss (2)
46-46: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
bdl-commonTypographystill violates kebab-case mixin naming.
commonTypographyis camelCase, same class ofscss/at-mixin-patternviolation flagged previously (that time forbdl-fontFamily--inter,bdl-commonTypography--dejaBlue, etc., which were subsequently renamed/restructured). This mixin name still fails the rule and will likely fail CI stylelint checks.🎨 Suggested rename
-@mixin bdl-commonTypography { +@mixin bdl-common-typography { `@include` bdl-fonts;Note: renaming requires updating call sites in src/components/dropdown-menu/DropdownMenu.scss and src/styles/common/_typography.scss.
🤖 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 `@src/styles/mixins/_typography.scss` at line 46, The mixin name bdl-commonTypography still violates the kebab-case scss/at-mixin-pattern rule because of the camelCase segment, so rename the mixin defined in bdl-commonTypography to a fully kebab-case name and update every reference to it. Make sure to adjust the call sites in DropdownMenu.scss and common/_typography.scss so they use the new mixin name consistently.
4-10: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
@at-root :where(body.is-inter-font) &never matches when included directly onbody.The same override pattern is repeated in
font-family(Lines 7-9),bdl-fonts(Lines 33-35), andbdl-commonTypography(Lines 53-55). Per the downstream call sitebody.is-deja-blue, body.is-inter-font {@includebdl-commonTypography; }(src/styles/common/_typography.scss:22-24),&resolves tobody.is-inter-fontitself, so the compiled selector becomes:where(body.is-inter-font) body.is-inter-font— an impossible ancestor/descendant match for a single<body>element. All three overrides (InterVariable swap,font-weight: 450,letter-spacing: 0) silently fail to apply at the primary body-level call site, even though they work correctly for non-body consumers like.dropdown-menu-elementorTooltip.A previous review flagged this exact issue and it was marked "✅ Addressed in commit 978a6bd," but the current code still shows the unified pattern with no body special-case, so the fix appears to be regressed or incomplete.
#!/bin/bash # Confirm call sites where these mixins are included directly on body-containing selectors rg -n -B2 -A2 'bdl-commonTypography|common-typography' src/styles/common/_typography.scssAlso applies to: 26-36, 46-56
🤖 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 `@src/styles/mixins/_typography.scss` around lines 4 - 10, The selector override pattern in font-family, bdl-fonts, and bdl-commonTypography still fails when the mixins are included on body selectors because `@at-root` :where(body.is-inter-font) & compiles to an impossible body-to-body descendant match. Update these mixins so they handle the body.is-inter-font call site from common/_typography.scss correctly, while preserving the existing override behavior for non-body consumers like dropdown-menu-element and Tooltip. Use the mixin names and the body-level include in bdl-commonTypography as the places to adjust the selector logic.
🤖 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.
Outside diff comments:
In `@src/elements/common/_variables.scss`:
- Line 3: The shared SCSS variables still have a remaining $font dependency in
ContentOpenWith.scss, so update the stylesheet that still references it to use
the new font variable or a local replacement before removing $font from
_variables.scss. Locate the usage in ContentOpenWith and any related shared
mixins/variables, then ensure the build no longer relies on $font anywhere.
---
Duplicate comments:
In `@src/styles/mixins/_typography.scss`:
- Line 46: The mixin name bdl-commonTypography still violates the kebab-case
scss/at-mixin-pattern rule because of the camelCase segment, so rename the mixin
defined in bdl-commonTypography to a fully kebab-case name and update every
reference to it. Make sure to adjust the call sites in DropdownMenu.scss and
common/_typography.scss so they use the new mixin name consistently.
- Around line 4-10: The selector override pattern in font-family, bdl-fonts, and
bdl-commonTypography still fails when the mixins are included on body selectors
because `@at-root` :where(body.is-inter-font) & compiles to an impossible
body-to-body descendant match. Update these mixins so they handle the
body.is-inter-font call site from common/_typography.scss correctly, while
preserving the existing override behavior for non-body consumers like
dropdown-menu-element and Tooltip. Use the mixin names and the body-level
include in bdl-commonTypography as the places to adjust the selector logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de1dd5ca-3684-40a3-829e-db62af7d69cd
📒 Files selected for processing (2)
src/elements/common/_variables.scsssrc/styles/mixins/_typography.scss
tjuanitas
force-pushed
the
feat-add-inter-font
branch
from
3fc6ace to
b1603f2
Compare
tjuanitas
force-pushed
the
feat-add-inter-font
branch
from
8832c55 to
480ceab
Compare
tjuanitas
force-pushed
the
feat-add-inter-font
branch
from
92cb1c0 to
0a1576e
Compare
tjuanitas
changed the title
Merge Queue Status
This pull request spent 11 minutes 20 seconds in the queue, including 11 minutes 5 seconds running CI. Required conditions to merge
|
3 participants
BREAKING CHANGE: renamed font-related SASS variables
bdl-fontSize=>bdl-fontSize--legacybdl-fontSize-minimum=>bdl-fontSize--minimumbdl-fontSize--dejaBlue=>bdl-fontSizeSummary by CodeRabbit