Add CSS Custom Properties to Protocol Themes #841

nathan-barrett · 2023-01-12T19:48:48Z

Description

This PR updates the themes in Protocol to use CSS Custom Properties over sass variable maps. To support legacy browsers, sass variables were created that will be used as a fallback. I've also added my notes to the bottom of the description if anyone needs a refresher

I have documented this change in the design system.
I have recorded this change in CHANGELOG.md.

Testing

To test that these changes are working I suggest using npm link(article on npm-link) since we haven't pushed any of the changes in protocol to the npm package. Directions below:

To test this works: (you will need a local version of both protocol and bedrock to test these changes)

In local protocol directory

npm run build-package (this will build the package)
cd package && npm link

In local bedrock

checkout the branch origin/protocol-update-test
npm link @mozilla-protocol/core (This will use the linked protocol package that was just built instead of using the NPM package)
add @import '~@mozilla-protocol/core/protocol/css/includes/themes'; to protocol-mozilla.scssandprotocol-firefox.scss`
npm run start
the server should start without any compilation errors, and there shouldnt be any style differences between prod and what you are viewing locally

Notes on Custom Properties

Blog Posts

How to create better themes with CSS variables - LogRocket Blog

Theming with CSS variables

Color Theme Switcher

Why we prefer CSS Custom Properties to SASS variables | CodyHouse

In SCSS:

$color-primary: hsl(220, 90%, 56%);

.link {
  color: $color-primary;
}

In CSS:

:root {
  --color-primary: hsl(220, 90%, 56%);
}

.link {
  color: var(--color-primary);
}

Differences between the two:

Custom Properties can be accessed at run time and stay in CSS code using --variable-name syntax that browsers recognize

you can preview all of the CSS variables in the browser

CSS custom properties can be accessed in JavaScript

// retrive the root element
const element = document.documentElement

// get variable from the inline style
element.style.getPropertyValue("--bg-color");
OR
// get variable from element
getComputedStyle(element).getPropertyValue("--bg-color");

// update variable value
element.style.setProperty("--bg-color", "orange");

CSS custom properties can accept a fallback value: var(--variable-name, fallbackValue)
Custom Properties are scoped to an element, most commonly the :root element

Theming with Custom Properties

Currently in protocol we use this function to retrieve variables depending on the theme:

// Get a theme variable from the collected map.
// This swaps the map based on the value of $brand-theme.
// Theme maps are defined in includes/_theme.scss

@function get-theme($var) {
    // Default to Firefox theme
    $theme-map: $theme-firefox;

    // Switch to Mozilla theme
    @if $brand-theme == 'mozilla' {
        $theme-map: $theme-mozilla;
    }

    @if not map-has-key($theme-map, $var) {
        @error '`#{$var}` is not a valid theme variable. ' +
								'Valid theme variable names: #{map-keys($theme-map)}';
    }

    @return map-get($theme-map, $var);
}

// and are used like this:

body {
    @include text-body-md;
    background: get-theme('background-color');
    color: get-theme('body-text-color');
    font-family: get-theme('body-font-family');
    line-height: type-scale('body-line-height');
}

With Custom properties, the variables would be set up like this:

// When using SASS variables in custom properties, 
// they need to be nested inside of #{}
:root {
	--bg-color: #{$color-blue};
	// etc etc...

	@if $brand-theme == 'mozilla' {
       --bg-color: #{$color-black};
  } @else if $brand-theme == 'firefox' {
       --bg-color: #{$color-orange};
  }
}

body {
    @include text-body-md;
    background: get-theme('background-color');
    color: get-theme('body-text-color');
    font-family: get-theme('body-font-family');
    line-height: type-scale('body-line-height');

    @supports (--css: variable) {
        background: var(--background-color);
        color: var(--body-text-color);
        font-family: var(--body-font-family);
				 line-height: var(--body-line-height);
    }
}

When used with #{} removes quotation marks from quoted strings, it’s sometimes necessary to wrap them in the meta.inspect() function to preserve quotes:

@use "sass:meta";

$font-stack-mozilla: 'Zilla Slab', Inter, X-LocaleSpecific, sans-serif;;

:root {
  --font-stack-mozilla: #{meta.inspect($font-stack-mozilla)};
}

reemhamz

What a long PR!
You did such a great job getting this done with almost zero nits and fixes!
Most of the nits I found were spelling errors in the markdown files, though.

GOOD JOB!!!!!!!!!!!!! 🕌

r+wc

assets/sass/protocol/includes/_mozilla.scss

assets/sass/protocol/includes/_themes.scss

docs/01-fundamentals/02-themes.md

docs/02-usage/02-framework.md

docs/01-fundamentals/02-themes.md

docs/02-usage/02-framework.md

maureenlholland

🎉 to echo Reem, this is a whole lot of great work. Really clean enhancement.

I did a code review (with a few todo notes to myself for tomorrow) and will do the local testing review tomorrow. Thought I'd put those comments out today for async discussion.

CHANGELOG.md

assets/sass/protocol/base/elements/_containers.scss

maureenlholland · 2023-02-15T16:47:15Z

assets/sass/protocol/components/_split.scss

@@ -129,7 +169,7 @@
    .mzp-c-split-body {
        @include border-box;
        @include bidi(((float, left, right),));
-        padding: 0 (get-theme('h-grid-lg') * 0.5);
+        padding: 0 ($h-grid-lg * 0.5);


curious: how is this working without calc?

lol! no clue, going to add calc in to just be extra sure it works.

🤷‍♀️ it works in both cases, no idea why in the first, seeing calc feels better for consistency 👍

maureenlholland · 2023-02-15T16:51:31Z

assets/sass/protocol/includes/_themes-sass.scss

@@ -6,6 +6,69 @@
 // themes. Mixins and functions can draw from these variable maps, swapping to
 // a different map based on the value of the global $brand-theme variable.

+// Default "theme" that will be served to legacy browsers -  IE 11 etc


👍 great comment

assets/sass/protocol/includes/forms/_lib.scss

maureenlholland · 2023-02-15T17:08:39Z

assets/sass/protocol/includes/themes/_index.scss

+        @extend %mozilla-theme;
+        @extend %mozilla-type-scale;
+
+        @if $type-scale == 'condensed' {


👏 I was thinking all the custom properties would be set on the :root inside the individual theme files but that would make the type scale logic trickier. This is an effective, readable way of breaking them up.

docs/01-fundamentals/02-themes.md

docs/02-usage/02-framework.md

theme/assets/sass/components/_type-specimen.scss

maureenlholland · 2023-02-15T17:47:46Z

assets/sass/protocol/includes/_lib.scss

@@ -44,5 +44,5 @@ $type-scale: 'standard' !default;
 // Import all the things!
 @import './node_modules/@mozilla-protocol/tokens/dist/index';
 @import 'functions';


curious: are we keeping the get-theme and type-scale functions in code for a while? I don't see them deleted (and I guess if they are not deleted, we don't have to make this a breaking change?)

I didn't want to delete them, since it would introduce a breaking change - but if we should go all out and remove them here I'd be happy to do so. @craigcook

Happy to leave as-is in this PR.

It might be worth keeping them for a while and adding a deprecation notice like we've done with some components, to give some transition time for anyone using those functions. But I'm not sure how widely they're used outside of Protocol itself... we only use get-theme in a few places in bedrock.

wonder how difficult it would be to write a migration script to do a search and replace (potentially with a flag setting for optional support query wrapping):

color: get-theme('title-text-color'); /* replace only */ color: var(--title-text-color); @include bidi(((padding-left, get-theme('h-grid-sm'), padding-right, get-theme('h-grid-sm')),)) /* replace & use support query */ @support (--css: variables) { @include bidi(((padding-left, var(--h-grid-sm), padding-right, var(--h-grid-sm)),)) }

I'm on the fence about including the support queries in end user code. I think it's important for clarity in Protocol, but since we've set all the IE defaults already, it should be safe for users to directly set custom property values on their style rules. Browser parsers that don't support custom properties will ignore those rules and use the Protocol defaults.

maureenlholland

A couple of minor bugs noted in the previous review

one more blocking issue from local testing of Protocol docs: the custom properties are not set in demos, i.e. http://localhost:3000/components/detail/paragraphs

local

live

I do think this counts as a breaking change if end users need to add a themes import.

If we want to try avoiding the duplicated :root rule declarations we ran into with @import, I started a POC here: f5b656b#diff-6da31b8d13d36754703085ab3c9ae106a4d80abb5f51e3a5db324042d4a9e63fR6

I ran into the "can't locate protocol tokens" error when trying to run a bedrock test. I think you had a solution for this, but I can't remember it. Happy to help try and debug that if we think it's worth pursuing.

maureenlholland · 2023-02-16T12:21:57Z

assets/sass/protocol/includes/themes/_mozilla.scss

+
+    // colors
+    --background-color-tertiary-inverse: #{$color-dark-gray-40};
+    --background-color-tertiary: #{$color-marketing-gray-20};


issue (blocking): mismatched custom property value with original moz theme value

Suggested change

--background-color-tertiary: #{$color-marketing-gray-20};

--background-color-tertiary: #{$color-light-gray-30};

https://github.com/mozilla/protocol/blob/main/assets/sass/protocol/includes/_themes.scss#L71

nathan-barrett · 2023-02-21T21:03:00Z

@maureenlholland made some fixes and tested in protocol docs and bedrock and things looked good to me

maureenlholland

👏 👏 👏 r+wc (mozilla tertiary background value is out of sync with prod)

My two cents (I think we discussed similar before) is we should bundle this change with a sass module update and be loud about a breaking release that brings us up to date with sass internally and creates new theming flexibility for Protocol users externally

maureenlholland · 2023-02-23T11:31:20Z

assets/sass/protocol/includes/themes/_mozilla.scss

+
+    // colors
+    --background-color-tertiary-inverse: #{$color-dark-gray-40};
+    --background-color-tertiary: #{$color-marketing-gray-20};


still need a minor fix here because we have a different mozilla tertiary value than in prod (I don't think anything is using this tertiary background at the moment, but it could cause confusion later)

maureenlholland · 2023-02-23T12:03:22Z

docs/02-usage/02-framework.md

-can have different values for different brands.
+the Mozilla and Firefox brands. Themes work by declaring a set of CSS Custom Properties
+on the document's root element (`:root`) in `/includes/_themes.scss` and are retrieved
+using the `var()` function. The custom properties will be updated depending on what theme is declared.


(non-blocking, just noting for a potential future docs update)
This is a very exciting change because we're not compiling away sass variables with internal functions anymore. We're using CSS inheritance. This allows end users more flexibility if the pre-defined Protocol themes don't apply (i.e. they are early days in the branding journey and want to experiment before creating an official Protocol theme).

We could provide a list of themeable CSS properties and demo how a user can define their own theme by declaring these properties after the theme declarations

/* user-lib.scss */ @use '~@mozilla-protocol/core/protocol/css/includes/themes'; /* user-defined theme values */ :root { --background-color-inverse: #{$color-green-80}; }

such a good idea!

nathan-barrett · 2023-02-23T17:01:02Z

Agree on bundling this with use and forward updates! Thanks for all of your help sifting through this @maureenlholland @reemhamz

maureenlholland · 2023-03-01T15:58:09Z

as discussed internally, waiting on a few non-breaking changes to create one more published version before creating a breaking change version

Had to also include an import in preview.scss since the demo's are inside a nested iFrame

alexgibson · 2023-07-21T08:35:51Z

When I run the protocol docs site locally, many of the components seem to be missing their theme styling?

alexgibson

I haven't gone as far as testing the bedrock changes, but it looks like things aren't quite working right on the protocol docs pages. I'm happy to take another look once things are working there.

I think we should also do some testing in legacy browsers (in bedrock) to make sure that the default theming is working as expected.

assets/sass/protocol/protocol.scss

nathan-barrett · 2023-07-21T16:42:17Z

@alexgibson - I fixed the doc site issue you mentioned (i think the theme import was removed during rebase). I will test thoroughly in legacy browsers with bedrock.

alexgibson

The code changes here look good to me and work well locally, nice work! r+

I do have one question though on something I'm not 100% clear on: at this point are we saying get-theme() and type-scale() should no longer be used?

If so, how are we planning to end of life their use? There are a couple of options I can think of:

We could warn people that they are deprecated and provide a migration guide, later removing the functions in a future major version bump?
We could remove the functions now and provide a guide, forcing a hard requirement for sites to remove their use of the functions before this next update.

Looking at mozilla/bedrock#13146 for example, I still see many calls to get-theme(), throughout bedrock. Does that mean there's likely pages using a mixture of both the new and old method for theming?

nathan-barrett · 2023-07-25T15:59:18Z

@alexgibson - I think option two makes the most sense. And I will write up a migration guide in the changelog that we can add when we push the next major version.

For bedrock, I would like to remove the refrences to the get-theme and get-type functions - just didn't do it in that PR.

alexgibson · 2023-07-25T16:35:22Z

Sounds good @nathan-barrett 👍

reemhamz self-requested a review January 16, 2023 16:22

reemhamz approved these changes Jan 19, 2023

View reviewed changes

nathan-barrett added the Do Not Merge ⚠️ Do Not Merge label Jan 25, 2023

nathan-barrett added Needs:Review 👋 Ready for Developer Review and removed Do Not Merge ⚠️ Do Not Merge labels Feb 2, 2023

maureenlholland reviewed Feb 15, 2023

View reviewed changes

maureenlholland suggested changes Feb 16, 2023

View reviewed changes

nathan-barrett requested a review from maureenlholland February 21, 2023 20:46

maureenlholland approved these changes Feb 23, 2023

View reviewed changes

maureenlholland added the Do Not Merge ⚠️ Do Not Merge label Mar 1, 2023

stephaniehobson mentioned this pull request Mar 2, 2023

No support for dark mode #647

Closed

craigcook removed the Do Not Merge ⚠️ Do Not Merge label Apr 18, 2023

nathan-barrett added 11 commits July 14, 2023 13:09

Add themes to root and separate files for mozilla and firefox

b5ecc83

Remove get-theme and replace with custom properties

fd1a13b

Remove type-scale references

22f7f03

Update typography and fix mistakes during conversion

cdffb9d

Update documentation for themeing

d6c70c0

update changelog

c050305

Fix typos in documentation

8bdf139

Update theme structure to fix bug

22770e9

Fix linting errors

29e8614

PR revisions and updates to fix broken styles in demos

a885eb1

Had to also include an import in preview.scss since the demo's are inside a nested iFrame

Fix typo with tertiary color

d5a172b

nathan-barrett force-pushed the themes-css-variables branch from a55c80a to 074fda5 Compare July 19, 2023 19:18

nathan-barrett mentioned this pull request Jul 19, 2023

Update protocol to v18.0.0 mozilla/bedrock#13146

Merged

2 tasks

nathan-barrett force-pushed the themes-css-variables branch from 074fda5 to ea2db68 Compare July 20, 2023 14:55

post-rebase fixes

edeb79b

nathan-barrett force-pushed the themes-css-variables branch from ea2db68 to edeb79b Compare July 20, 2023 22:21

nathan-barrett removed the Needs:Review 👋 Ready for Developer Review label Jul 20, 2023

alexgibson requested changes Jul 21, 2023

View reviewed changes

assets/sass/protocol/protocol.scss Outdated Show resolved Hide resolved

nathan-barrett requested a review from alexgibson July 21, 2023 16:31

Add theme to doc site preview

816ddd4

nathan-barrett force-pushed the themes-css-variables branch from dca3ec9 to 816ddd4 Compare July 21, 2023 16:34

alexgibson approved these changes Jul 24, 2023

View reviewed changes

Add migration guide on changelog

209c14d

nathan-barrett merged commit e244b27 into main Jul 25, 2023
2 checks passed

nathan-barrett deleted the themes-css-variables branch July 25, 2023 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CSS Custom Properties to Protocol Themes #841

Add CSS Custom Properties to Protocol Themes #841

nathan-barrett commented Jan 12, 2023 •

edited by alexgibson

Loading

reemhamz left a comment

maureenlholland left a comment

maureenlholland Feb 15, 2023

nathan-barrett Feb 17, 2023

maureenlholland Feb 23, 2023

maureenlholland Feb 15, 2023

maureenlholland Feb 15, 2023

maureenlholland Feb 15, 2023

nathan-barrett Feb 21, 2023

maureenlholland Feb 23, 2023

craigcook Feb 23, 2023

maureenlholland Feb 24, 2023

maureenlholland left a comment

maureenlholland Feb 16, 2023

nathan-barrett commented Feb 21, 2023

maureenlholland left a comment

maureenlholland Feb 23, 2023

maureenlholland Feb 23, 2023 •

edited

Loading

nathan-barrett Feb 23, 2023

nathan-barrett commented Feb 23, 2023 •

edited

Loading

maureenlholland commented Mar 1, 2023

alexgibson commented Jul 21, 2023

alexgibson left a comment

nathan-barrett commented Jul 21, 2023

alexgibson left a comment

nathan-barrett commented Jul 25, 2023

alexgibson commented Jul 25, 2023

	--background-color-tertiary: #{$color-marketing-gray-20};
	--background-color-tertiary: #{$color-light-gray-30};

Add CSS Custom Properties to Protocol Themes #841

Add CSS Custom Properties to Protocol Themes #841

Conversation

nathan-barrett commented Jan 12, 2023 • edited by alexgibson Loading

Description

Testing

Notes on Custom Properties

Theming with Custom Properties

reemhamz left a comment

Choose a reason for hiding this comment

maureenlholland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maureenlholland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathan-barrett commented Feb 21, 2023

maureenlholland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maureenlholland Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathan-barrett commented Feb 23, 2023 • edited Loading

maureenlholland commented Mar 1, 2023

alexgibson commented Jul 21, 2023

alexgibson left a comment

Choose a reason for hiding this comment

nathan-barrett commented Jul 21, 2023

alexgibson left a comment

Choose a reason for hiding this comment

nathan-barrett commented Jul 25, 2023

alexgibson commented Jul 25, 2023

nathan-barrett commented Jan 12, 2023 •

edited by alexgibson

Loading

maureenlholland Feb 23, 2023 •

edited

Loading

nathan-barrett commented Feb 23, 2023 •

edited

Loading