-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: design tokens ADR #1929
docs: design tokens ADR #1929
Conversation
✅ Deploy Preview for paragon-openedx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #1929 +/- ##
==========================================
+ Coverage 90.73% 91.38% +0.64%
==========================================
Files 233 234 +1
Lines 4082 4157 +75
Branches 966 1001 +35
==========================================
+ Hits 3704 3799 +95
+ Misses 371 351 -20
Partials 7 7 ☔ View full report in Codecov by Sentry. |
This is super interesting. I have a few questions:
CC @xitij2000 |
Thanks for the questions, @bradenmacdonald (responses inline):
I've included more details about different types of tokens, abbreviated examples of JSON snippets, and a link to all the current tokens as they exist currently in the
Yes, design tokens are only the "key/value pairs" of a token name (key) to a style property (value). Similar to our SCSS variables today, design tokens themselves don't really define how the resulting CSS variables are used or applied to the theme and UI components via CSS rules. That would come elsewhere, e.g. the core Paragon theme using the resulting output CSS variables or a On a related note, though, we've often historically treated Paragon's class names (e.g.,
Good distinction. I believe you're getting at the difference between system-wide theming and what I'm calling "organizational" theming in the ADR. I've added some additional thoughts around this distinction to the ADR and some forward thinking (likely out of scope for the initial ADR?) ideas on how to enable broader support for organizational themes. For what it's worth, in the Enterprise MFEs (frontend-app-admin-portal & frontend-app-learner-portal-enterprise), we do currently support (in an admittedly somewhat hacky way) dynamic user-driven custom themes picked and/or created by Enterprise Admins, so we'll definitely be interested in finding a solution here for this as well. Let me know if the additions to the ADR helps address these questions 😄 |
@adamstankiewicz Thanks for the excellent additions to this ADR; that helps give me a much clearer picture here and answered most of my questions. 💯
Ah, we actually have both use cases. Even for the "system-wide" case, we'd like it to have a much shorter iteration cycle. Imagine that we host 100 Open edX instances and we want each of those 100 customers to be able to customize their theme in our admin portal. They're each doing "system-wide" customizations for their entire LMS+Studio, but we'd like them to be able to do that theming themselves and see the results immediately. Right now, they can go into our hosting admin panel to choose their colors etc. and we can show a "fake" preview of how it will look (a mock up), but it takes quite some time to actually apply their changes and roll it out to their instance before they can see in their actual LMS / MFE (which we do using simple-theme). Our long-term ambition is to have a theme management tool built into the Open edX platform somewhere that can adjust colors and show the results immediately without a build process. |
@bradenmacdonald The current process to apply their changes and roll it out to their instance is predominantly slow because of our dependence on SCSS which requires all consuming applications (e.g., 30 MFEs) to get re-built on any theme change. The intent of this ADR is to take a (large) incremental step towards solving that problem by at least only requiring the theme to re-built rather than all its consuming applications whenever changes are made to the theme by migrating to CSS variables. To keep scope down for the initial release, the goal of this particular ADR is largely around the migration from current state (SCSS) to desired state (CSS variables via design tokens). Doing this incremental step is foundational in that Paragon can support runtime theming at all. That said, we are planning and hoping for this approach to be extended in several ways moving forward in the future, including to achieve the long-term ambition described below:
This solution you're describing is one we have at least 3 known use cases for currently, so we'll definitely need to find a generic/flexible solution here for managing the Paragon theme. My current thinking to enable support for these use cases might be to additionally expose the A benefit of this approach is that consumers can take advantage of the A secondary benefit is that consumers could also pass their own custom JSON tokens to the Node.JS API (i.e., not overriding core Paragon tokens), and // 3 custom brand colors picked by an administrator in a UI
const brandColors = {
primary: '#BB4430',
secondary: '#003366',
tertiary: '#F3E9DC',
};
const response = await fetch('http://localhost:3000/tokens', {
method: 'POST',
headers: {
Accept: 'application/json',
'Content-Type': 'application/json',
},
body: JSON.stringify({
color: {
primary: {
base: {
value: brandColors.primary,
},
},
hero: {
bg: {
value: brandColors.secondary,
},
text: {
value: '{color.hero.bg}',
modify: [{ type: 'color-yiq' }],
},
border: {
value: brandColors.tertiary,
},
},
},
}),
});
const json = await response.json();
console.log(json); The output of {
"success": true,
"cssOutput": ":root { /* contains all Paragon's generated CSS variables based on the custom tokens sent in the POST payload above */ }"
} Then, MFE could use something like <Helmet>
<style type="text/css">{brandStyles.cssOutput}</style>
</Helmet> While such a Node.js API that combines custom tokens with Paragon's core tokens to give CSS variables output solves the application of custom theme changes in real time, it does not solve how those custom theme changes would persist (e.g., in a database). While we have started to think along the lines of the "real time" theming, I'd say it is out of the scope for this initial ADR, but should definitely be on the roadmap. We'd love to collaborate with Open Craft to iterate and build out such a solution for the true real-time / live theming use case, perhaps with the approach described above or otherwise. |
I was just replying about below comment, the above was posted :-)
The build step from a tokens file to built css is under 1 second, and I think for live-previewing I'd like to see if we can improve that by only building a subset i.e. if you're changing variables related to the theme, then only build tokens for that, and patch those variables. As suggested above, we also have the theme UI output tokens, and when the theme is saved, it can rebuild and redeploy the theme CSS. At that point having a common theme is less important from a real-time theming perspective since only the variables need to be reloaded, and for that we can have a temporary preview mechanism that can use a live-updating theme css used in place of a pre-build variables file. Switching between variable files at runtime can then also enable multiple (light/dark/high-contrast) themes. |
|
||
By only transforming our hardcoded SCSS variables to hardcoded CSS variables, we would be missing out on the opportunity to iterate towards the vision of making the Paragon design system be platform-agnostic. | ||
|
||
As a result, Paragon's existing SCSS variables will be migrated to design tokens defined as JSON files that get transformed by ```style-dictonary``` into various platform-specific styles. To start, we are transforming the design tokens specified in JSON to CSS variables as well as some CSS utility classes. In the future, our approach may expand to transforming the design tokens to iOS and Android compatible files as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there is a typo "dictonary". Also in the next line
@adamstankiewicz thanks a lot for this excelent ADR. I just now learned about the style-dictionary but I can already see how this will have a great impact in getting the first level of MFE customization off the ground. +1 on this decision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super exciting! I'm really looking forward to sinking my teeth into it a bit and getting design tokens up and running on an MFE!
* The primary theming use case for Paragon is largely around system-wide theming, where all applications in the Open edX ecosystem share the same theme. | ||
* However, there are use cases for organizational themes, too (i.e., updating the colors for specific partners/organizations, enterprise customers, etc.). | ||
|
||
* This is not well supported today and largely requires overriding CSS classes from Paragon rather than the desired approach of overriding underlying CSS variable(s). This an anti-pattern as Paragon class names should really be considered internal implementation details of Paragon components, and not used by consumers directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have an example of organizational theming being done the "anti-pattern way" vs the "design token way" to demonstrate this.
I'm not sure where exactly the specific example could fit into the ADR, but I think this is an incredible illustration of why design tokens/css variables are such a great approach here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brian-smith-tcril I'll give a go at including a brief example of the Enterprise MFEs relying on overriding class names where it would instead be great for those Enterprise MFEs to rely on the design tokens tooling / CSS variables instead for a more complete/holistic organization theming solution 👍
Let me know what you think of the following verbiage I just added to this paragraph:
For example, the Enterprise MFEs within Open edX (e.g., frontend-app-learner-portal-enterprise) inject
<style>
tag in the HTML document with CSS class name overrides such as.btn-primary { background-color: $enterpriseCustomerPrimaryColor }
. Ideally, similar use cases could instead override a CSS variable such as--pgn-color-btn-primary-bg
instead such that it gets re-used throughout all Paragon styles, not just.btn-primary
.
|
||
Currently, Paragon recommends theme authors to create a theme package such as ``@edx/brand-openedx`` (`Github <https://github.com/openedx/brand-openedx>`__) and ``@edx/brand-edx.org`` (`Github <https://github.com/edx/brand-edx.org>`__). | ||
|
||
While the migration from SCSS variables to CSS variables is a breaking change for theme authors, we have tried to mitigate this by keeping the existing SCSS variables but defining them such that their values refer to the new CSS variables. Because SCSS can't evaluate the CSS variable at runtime, it utilizes the CSS variable in the resulting output CSS used in the browser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of me feels like it might be better to "rip the band-aid off" and directly use css vars everywhere scss vars were being used. My main worry is having a mix of scss vars and css vars scattered throughout stylesheets preventing a true "source of truth" for a value.
For example, let's say we have my-scss-var-foo
set to my-css-var-foo
:
- if all the stylesheets use
my-scss-var-foo
, and never usemy-css-var-foo
directly, changingmy-scss-var-foo
to bemy-css-var-bar
would work as expected. - howerver, if some stylesheets use
my-scss-var-foo
and some usemy-css-var-foo
, changingmy-scss-var-foo
could lead to unexpected behavior.
I can think of 2 possible ways to prevent this problem:
- Watch PRs that change scss vars like a hawk, don't let it happen
- Only use css vars
I know there was probably a lot of discussion/thought put into this decision, I'd love to hear any thoughts (or if there was a pros/cons list) that came up when building towards this strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brian-smith-tcril I agree, this should likely be revisited. We did "rip the b(r)and-aid off" (pardon the terrible pun with the (r) haha!) in @edx/paragon
itself by updating all the general theme and component styles to no longer use the SCSS variable in favor of using the CSS variable directly (e.g., see Alert
styles).
However, Paragon also does still have the _variables.scss
file around in the alpha
release, where each of the SCSS variables point to the associated CSS variable. I agree this isn't ideal for maintainability/understandability (e.g., single source of truth). Would you agree that we should likely delete _variables.scss
in the design tokens release, if possible, at this point? Consumers should no longer be importing anything from Paragon's SCSS directly anymore but would need to verify that the build-scss
script in Paragon doesn't rely on _variables.scss
.
Future considerations: Customizing the theme via a user interface | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
More forward thinking, we would also like to explore creating a UI on top of these design tokens such that the understanding and writing of JSON files is abstracted away from theme authors. Such a UI may also enable designers to self-serve update the theme. | ||
|
||
Though this theme customization UI is not included in the initial release of design tokens and CSS variables, there is desire to do some prototyping to see what might be possible; other groups in the community may also have the capacity to run with it as well. | ||
|
||
That said, such UI considerations thus far have largely been for theme authors at the system/provider level, not so much at the user level. It may be interesting to explore whether Paragon could (and/or should) expose some generic and flexible helper components, hooks, functions, etc. that consuming applications could utilize to simplify the creation and injection of a dynamic, user-driven theme's CSS variables. | ||
|
||
As a more concrete example, consuming applications could, in theory, use an exported function from Paragon that accepts a list of JSON and/or JavaScript objects as design tokens (similar to importing all the token files in the tokens build) and then run ``style-dictionary`` with the same (or extended) config on these custom tokens and the core Paragon tokens to generate the dynamic CSS variables. This solution, too, is still pretty raw and is likely out of scope of the initial design tokens release and this ADR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCraft have an open source UI for customising themes that currently uses edx/configuration which generates SCSS variables, but we were planning on adapting it in the future to be a standalone MFE for updating the theme using whatever mechanism is adopted for runtime theming.
We'd love to collaborate on this, and potentially make it a kind of first-setup/onboarding MFE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think there's a couple different related use cases here.
As you're describing, I think, there could be a UI for customizing themes/tokens and generating resulting CSS files on disk. I believe there may also be use cases for needing to expose the style-dictionary
tooling from Paragon as an API (e.g., on a Node.js Express server) that may be called at runtime to generate CSS variables based on custom token values stored in a database. In this case, since the application needs to generate CSS variables with custom design tokens at runtime, I believe the only way (afaik) to get MFEs to run style-dictionary
is through a Node.js-based API.
I will also mention the API-based use case in this paragraph.
|
||
A critical component of the Open edX platform is the ability to customize its visual styles to reflect the custom brand of its consumers in the Open edX community. Historically, the Open edX platform (via ``edx-platform``) has supported a comprehensive theming system fulfilling the community's theming needs, including brand customization but also functionality, too (e.g., modifying, adding, or removing user interface elements). However, as we've moved towards React micro-frontends, the theming from a brand customization perspective has been largely replaced by the theming system provided by the Paragon design system. | ||
|
||
Within Paragon, "theming" predominantly refers to brand customization as it relates to visual styles. It does not intend to be responsible for customization of functionality as in the historical sense of comprehensive theming in ``edx-platform``. Enabling such customization to bring comprehensive theming support to micro-frontends is on the roadmap for the Frontend Working Group as a separate initiative outside of Paragon itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling such customization to bring comprehensive theming support to micro-frontends is on the roadmap for the Frontend Working Group as a separate initiative outside of Paragon itself.
I'd be happy if this sentence weren't there. 😂 I feel pretty strongly that comprehensive theming is not a thing we want to promote or enable for micro-frontends; we should find less invasive mechanisms for doing common theming tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidjoy [inform] I updated this sentence to say, "The Frontend Working Group's roadmap includes improvements to micro-frontend customizability.", without an explicit mention of assuming parity to comprehensive theming.
Future considerations: Customizing the theme via a user interface | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
More forward thinking, we would also like to explore creating a UI on top of these design tokens such that the understanding and writing of JSON files is abstracted away from theme authors. Such a UI may also enable designers to self-serve update the theme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[naive question] Is there a way for Figma to be a potential source of truth for the design tokens via some sort of export? Just thinking about designers getting to use a powerful industry standard tool they're familiar with vs. a tool we have to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this is definitely part of the long term vision! For example, the Tokens Studio plugin for Figma has a GitHub Sync feature that could enable a two-way sync between design tokens defined in code and Figma, where designers could even self-serve update the value of design tokens and see their changes propagate throughout the whole system with minimal engineering effort (e.g., changing a single color across the platform).
I'm surprised this wasn't included in the example potential benefits here! I will include some words about this.
Challenges with current styles architecture | ||
------------------------------------------- | ||
|
||
* **On theme changes, all its consuming applications must be upgraded, re-built, and re-deployed.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this ADR encompass a mechanism for runtime updates to the theme? Does that imply that paragon is deployed somewhere and that MFEs reference its deployed stylesheet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is a separate chunk of work in frontend-platform (PR) to take the resulting already compiled CSS from Paragon's design tokens and ingest them as external CDN urls into MFEs based on the MFE's configuration.
With this approach, once the configuration is updated (which may be done via runtime MFE config API, without requiring any re-deploys), updates to the Paragon styles could automatically get pulled in.
For initial MVP release, we plan on relying on jsDelivr, an open-source CDN heavily used in the JS ecosystem, to host the @edx/paragon
and @edx/brand
compiled CSS for MFEs to import.
I'm late to the party, admittedly. There's a lot of good stuff here - what do we think we need to do in order to approve and merge this ADR? My impression is that there's a lot of forward-looking vision statements in this ADR, I wonder if one path forward is to pare it down to its core that we intend to move forward with now, given that this may be a long, multi-phased effort. It may also be that we need a plan for following up on this ADR and doing the work (apologies for not being up on any prior art there). Are we able to enumerate the areas of this ADR that still require some discussion? |
I was also wondering what was missing for this PR to be approved and merged. We have been building on top of this concept and so far we are happy with the results. After that we add the css variables file to the mfe_config API. like so: "MFE_CONFIG": {
"PARAGON_THEME_VARIANTS_LIGHT_URL": "https://css-varsify.s3.amazonaws.com/public/0e61a3ff-4ec7-4246-baf2-b9cb8eee6d8d.css"
}, The css variables is built with different strategies. One being paragon. We have also built it with sytle-dict and we even have a django_template for rendering it on the go. This site only has the learning mfe configured to use paragon-alpha (that's the picture above), but if there is interest in see it in action by someone else, I could build more of the mfes. |
To all reviewers thus far, I'd love to get the spider webs brushed off this ADR and get it merged into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We've been working on design tokens for quite a while now and I think @adamstankiewicz does an excellent job summing it all up in this ADR, thank you for doing that! I couldn't have written this better myself, event though I spent a lot of time on making design tokens work in Paragon 🙂
Considering that we are really close to the release of the design tokens work, I really feel like this ADR should finally get merged.
Co-authored-by: Felipe Montoya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Really looking forward to being able to build on top of this!
I've played around with the alpha builds and, I think, this summarizes everything well.
Thanks for the work on this!
🎉 This PR is included in version 20.45.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Drafts an ADR about design tokens,
style-dictionary
, and runtime theming.Preview
https://github.com/openedx/paragon/blob/005c873e3563389f7dfa02e1414909ce39fac3a4/docs/decisions/0019-scaling-styles-with-design-tokens.rst
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist