Conversation
Introduce shared font-size variables and new fixed-size caption mixins (type-caption-small/medium/large) in _typography.module.scss. Replace scattered literal px font-size usages in select, tooltip, slider_time and data_chart_choropleth styles with the new type-* mixins to standardize typography and ensure consistent responsive behavior where applicable. This is a styling refactor with no functional changes.
Introduces a new mobile breakpoint token and wires it into the shared breakpoint map. This also updates the typography scale so headline, body, and label styles use smaller base sizes on mobile and increase at the appropriate tablet/laptop/desktop breakpoints.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the typography system by introducing a private font-size scale, adding a mobile breakpoint, and replacing literal font sizes with mixins across several component stylesheets. However, the review identified critical regressions in the typography mixins: the base font-sizes for both 'type-headline' and 'type-body-large' were accidentally removed, which will cause layout issues on mobile viewports. Additionally, the updated scales for 'type-label-large' and 'type-label-extra-large' break the typography hierarchy by making them smaller than their medium counterparts.
Update typography scale in _typography.module.scss: set headline base font-size to $-font-size-16 and body base to $-font-size-14. Adjust label sizes: type-label-large from 12→14 (tablet 14→16) and type-label-extra-large from 14→16 (tablet 16→18, laptop 18→20).
nick-nlb
left a comment
nick-nlb
left a comment
There was a problem hiding this comment.
Thank you Pablo! This is looking very nice.
The PR omits a high level description of what actually changed. For example, this changed font sizes across the app (including the home page, toast, etc).
Can we update the overview to include that high level description.
There are some other comments in the review, and then this should be ready!
|
|
||
| @mixin type-caption-medium($font-weight: 400) { | ||
| font-size: $-font-size-13; | ||
| font-weight: $font-weight; |
There was a problem hiding this comment.
Should this use the weight guard -font-weight-google-sans?
| font-size: 11px; | ||
| font-weight: 600; | ||
|
|
||
| @include type-caption-small(600); |
There was a problem hiding this comment.
Relating to the comment about the guard, 600 not an available font weight.
| @use "@package/tokens/scss" as tokens; | ||
|
|
||
| $-breakpoints: ( | ||
| "mobile": tokens.$breakpoint-mobile, |
There was a problem hiding this comment.
The PR description notes in follow-ups that this isn't being used.
If it isn't, can we remove it, and add it in when it is used later in another PR?
| // Shared font-size scale backing the 'type-*' mixins below. Private to this | ||
| // module - we should always reach for a 'type-*' mixin instead of a | ||
| // literal px value or one of these variables directly. | ||
| $-font-size-10: 10px; |
There was a problem hiding this comment.
What is the difference between using these variables and using the pixel?
I.e., $-font-size-11 could never be any other pixel size than 11px without being wrong.
|
|
||
| // Fixed-size (no breakpoint scaling), for dense chart/control UI text that | ||
| // shouldn't grow on larger viewports. | ||
| @mixin type-caption-small($font-weight: 400) { |
There was a problem hiding this comment.
What does caption specifically refer to here. It makes sense to me to have a font role family semantically named (like captions) but we should have clarity (embedded into AGENTS.md) about how to select the font for a given purpose. For example in tooltips we use "caption" for empty, but hint uses body small.
| min-height: 240px; | ||
| font-size: 13px; | ||
|
|
||
| @include type-caption-medium; |
There was a problem hiding this comment.
I think we usually put these at the top of the block (the includes) with a space underneath them. Can you double-check and then adjust to convention if need be? If this is the convention (includes at the top of block) can we document it FRONTEND.md
nick-nlb
changed the title
This branch has not been deployed
2 participants
Overview
Consolidates chart/select component font sizes onto the shared type-* typography mixins instead of hardcoded px values, and extends the typography scale with a mobile breakpoint token (480px) plus new type-caption-* mixins for fixed-size chart/control UI text that shouldn't scale up on larger viewports.
Changes Made
Testing Done
Describe the steps you took to test these changes, listing the exact commands
run and reproducible steps a reviewer can follow.
Risk & Rollback
Low risk — purely presentational (font-size only), no logic or API changes. Worst case is a visual size regression on the touched components/breakpoints; revert this PR to roll back, no data or config migration involved.
Follow-ups
The new mobile breakpoint token is added but not yet @included anywhere directly — current mixins rely on their unwrapped base styles as the mobile-first floor. None tracked separately; flag if a dedicated mobile override is needed later.
Checklist
AGENTS.mdand followedCODING_GUIDELINES.md, plusFRONTEND.mdfor UI changes.that application's guide.
Note: Only Maintainers can approve and merge PRs. Expected initial review
time: 3 business days.