Refactor color to use CSS custom properties (#982) #1109
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephaniehobson
force-pushed
the
color-vars
branch
6 times, most recently
from
4d0cd19 to
9934957
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the color system to use CSS custom properties as the primary method for applying colors, following the earlier typography refactor. The changes modernize the color implementation by defaulting to CSS variables, reorganize the variable naming structure with --theme- and --token- prefixes, and remove now-unnecessary @supports blocks.
Changes:
- Migrated all color values from Sass variables to CSS custom properties with
--theme-prefix - Reorganized color token definitions into new
root/_color.scss,root/_theme.scss, androot/_spacing.scssfiles - Removed
@supportsblocks that only contained color declarations since CSS custom properties are now required
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| theme/assets/sass/theme.scss | Updated import path from themes to root |
| theme/assets/sass/settings/_settings.scss | Added Fractal-specific CSS custom properties for theme colors |
| theme/assets/sass/preview.scss | Updated import path from themes to root |
| theme/assets/sass/components/_type-specimen.scss | Replaced Sass color variable with CSS custom property |
| theme/assets/sass/components/_tree.scss | Replaced Sass color variables with Fractal CSS custom properties |
| theme/assets/sass/components/_search.scss | Replaced Sass color variables with Fractal CSS custom properties |
| theme/assets/sass/components/_prose.scss | Replaced Sass variables with CSS custom properties and removed @supports blocks |
| theme/assets/sass/components/_preview.scss | Replaced Sass color variable with CSS custom property |
| theme/assets/sass/components/_pen.scss | Replaced Sass variables with CSS custom properties and removed @supports blocks |
| theme/assets/sass/components/_navigation.scss | Replaced Sass color variable with CSS custom property |
| theme/assets/sass/components/_header.scss | Replaced Sass variable with CSS custom property and removed @supports block |
| theme/assets/sass/components/_document.scss | Replaced Sass variable with CSS custom property and removed @supports block |
| theme/assets/sass/base/_typography.scss | Replaced Sass variable with CSS custom property and removed @supports block |
| docs/02-usage/migration.md | Added color migration instructions including find/replace patterns |
| assets/sass/protocol/root/_typography.scss | Extracted theme and spacing definitions to separate files |
| assets/sass/protocol/root/_theme.scss | New file containing theme configuration logic |
| assets/sass/protocol/root/_spacing.scss | New file containing spacing grid variable definitions |
| assets/sass/protocol/root/_index.scss | New index file importing color, theme, spacing, and typography modules |
| assets/sass/protocol/root/_color.scss | New file defining theme-to-token color mappings |
| assets/sass/protocol/protocol.scss | Added import for new root module |
| assets/sass/protocol/includes/themes/_mozilla.scss | Renamed color variables with --token- prefix and added status colors |
| assets/sass/protocol/includes/themes/_firefox.scss | Renamed color variables with --token- prefix and added status colors |
| assets/sass/protocol/includes/mixins/_utils.scss | Replaced Sass variables with CSS custom properties in link mixins |
| assets/sass/protocol/includes/forms/index.scss | Moved form color variables to CSS custom properties in :root block |
| assets/sass/protocol/includes/_themes-sass.scss | Removed Sass color variables (now using CSS custom properties) |
| assets/sass/protocol/components/forms/_status.scss | Replaced Sass variables with CSS custom properties |
| assets/sass/protocol/components/forms/_msg.scss | Wrapped arithmetic operation in calc() |
| assets/sass/protocol/components/forms/_form.scss | Replaced Sass variables with CSS custom properties and removed @supports blocks |
| assets/sass/protocol/components/forms/_choice.scss | Replaced Sass variables with CSS custom properties throughout |
| assets/sass/protocol/components/_sticky-promo.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| assets/sass/protocol/components/_sidebar-menu.scss | Replaced Sass color variable with CSS custom property |
| assets/sass/protocol/components/_notification-bar.scss | Replaced Sass color variables with theme status color CSS custom properties |
| assets/sass/protocol/components/_navigation.scss | Replaced Sass color variables with CSS custom properties |
| assets/sass/protocol/components/_modal.scss | Replaced Sass color variables with CSS custom properties |
| assets/sass/protocol/components/_menu.scss | Replaced Sass color variables with CSS custom properties |
| assets/sass/protocol/components/_menu-list.scss | Replaced Sass color variables with CSS custom properties |
| assets/sass/protocol/components/_menu-item.scss | Replaced Sass color variables with CSS custom properties |
| assets/sass/protocol/components/_footer.scss | Replaced Sass variables with CSS custom properties, keeping tokens namespace for specific grays |
| assets/sass/protocol/components/_feature-card.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| assets/sass/protocol/components/_emphasis-box.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| assets/sass/protocol/components/_card.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| assets/sass/protocol/components/_button.scss | Replaced Sass color variables with CSS custom properties |
| assets/sass/protocol/components/_breadcrumb.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| assets/sass/protocol/base/utilities/_rich-text.scss | Kept tokens namespace for specific background color |
| assets/sass/protocol/base/utilities/_backgrounds.scss | Replaced Sass variables with CSS custom properties and removed @supports blocks |
| assets/sass/protocol/base/elements/_titles.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| assets/sass/protocol/base/elements/_quotes.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| assets/sass/protocol/base/elements/_links.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| assets/sass/protocol/base/elements/_forms.scss | Replaced Sass variables with CSS custom properties throughout form elements |
| assets/sass/protocol/base/elements/_document.scss | Replaced Sass variables with CSS custom properties and removed @supports block |
| CHANGELOG.md | Documented color refactor changes and migration instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| box-shadow: forms.$field-focus-ring; | ||
| color: #222; | ||
| box-shadow: var(--theme-field-focus-ring); | ||
| color: var(--theme-body-text-color); |
There was a problem hiding this comment.
The hardcoded color value #222 was replaced with var(--theme-body-text-color), but #222 is a dark gray, not pure black. Verify that --theme-body-text-color provides the intended visual result in this context.
Suggested change
| color: var(--theme-body-text-color); | |
| color: #222; |
Contributor
Author
There was a problem hiding this comment.
I'm not sure why this difference existed before or even why we are explicitly declaring it here. I'm going to leave the declaration incase it solved a problem.
However, I believe the text in the select box should match the text colour of the other form components and this does.
stephaniehobson
force-pushed
the
color-vars
branch
from
9934957 to
81337f1
Compare
Refactor color system to use CSS custom properties. This is the second PR in a refactor of how we use CSS variables (#982), following the typography refactor (#1107). Some kind of basic fallback support for older browsers will be added back in a later PR once we can get a clear picture of how broken things are. I know this looks big but for the most part it's just a straight find/replace on variable names or moving @includes to the end of declarations. Modernization: Default to CSS vars for colors, backgrounds, and borders Removed @supports declarations if they only had color declarations Color mixins now use CSS vars Reorganization: Added --theme- prefix to variables expected to morph Added --token- prefix to unchanging variables Created root/_color.scss for color token definitions Created root/_theme.scss for theme color mappings Created root/_spacing.scss for spacing tokens Moved theme definitions to includes/themes/ Bug fixes: Moved @include statements to end of declarations
stephaniehobson
force-pushed
the
color-vars
branch
from
81337f1 to
5993295
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Refactor color system to use CSS custom properties. This is the second PR in a refactor of how we use CSS variables (#982), following the typography refactor (#1107). Some kind of basic fallback support for older browsers will be added back in a later PR once we can get a clear picture of how broken things are.
I know this looks big but for the most part it's just a straight find/replace on variable names or moving
@includesto the end of declarations.Modernization:
@supportsdeclarations if they only had color declarationsReorganization:
Bug fixes:
@includestatements to end of declarationsNote:
I left Sass variables in place in places where they referenced a specific colour and I didn't think it was appropriate to try to theme them yet. I think it's okay to try to address those in a later PR focused on adding dark mode support.
Issue
#982
Testing
Spot check comparison to protocol.mozilla.org. (Especially pages with interactive stuff like the modal page, that's not covered by the visual regression tests.)
--
CHANGELOG.md.