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.
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: ADR to expose all SCSS variables that MFEs use as CSS variables [BB-6307] #1388
docs: ADR to expose all SCSS variables that MFEs use as CSS variables [BB-6307] #1388
Changes from all commits
9a4fb3f
6f4e1d4
f7a7024
ec17bd0
62842d0
7c87075
d711d2e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm in full support of this ADR and decision. As you mentioned, we can start moving towards CSS variables in Paragon since Open edX officially no longer supports older browsers like IE 11, per our shared
@edx/browserslist-config
.There are still some open questions around the Bootstrap 5 upgrade, which is why it hasn't necessarily been prioritized yet. I agree with the approach to namespace all the Paragon specific CSS variables with
--pgn
so they are distinct from the CSS variables provided by Bootstrap, if/when we do upgrade.For what it's worth, adopting CSS variables is on the Paragon Working Group team's roadmap, likely in a few weeks, as part of our effort to adopt a more scalable solution to design tokens, using something like
style-dictionary
(demo), where design tokens are drafted as JSON files in a standard format, and thenstyle-dictionary
transforms/exports those design tokens into various other formats accepted by various platforms (e.g., JS, CSS variables, SCSS, iOS, Android, Figma, Sketch, etc.). We are likely planning to build out a prototype of this concept for design tokens in the next month or two through the Blended Development project BD-39 that I'm leading with Raccoon Gang.What is the timeline you're looking for here in an implementation? Were you planning on making the contribution to Paragon to create these CSS variables yourself? If so, can you expand on this decision in your ADR to help explain the plan for how we'll manage the creation of these CSS variables in more detail?
I'm planning to draft an ADR for Paragon about
style-dictionary
that refers to this PR, as a way to implement design tokens and automation around generation of CSS and SCSS variables (among other platforms 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.
@adamstankiewicz
I am definitely interested in contributing towards this myself, and taking help from others from OpenCraft. I'll expand on how these SCSS variables can be created in this ADR to explain my proposal for how we can get started.
Last I read up about experiments with css in JS my understanding was that that approach was abandoned. The
style-dictionary
approach seems a bit different. Is there any place I can read more about how it would be used for MFEs?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.
I read up on style-dictionary the use case that they are trying to solve is maintaining the constant style across platforms while our need is to make the style changes independent of the build process, right?
The other point I have is the need to use CSS variable will help us to reduce/eliminate the need to use SCSS preprocessor. While if we try to migrate/use style-dictionary we will again fall into the parser trap. Where our build time is dependent on how fast the parser is.
Do we really need that?
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.
I don't think eliminating SCSS is a goal here. Even if SCSS takes time to build it does provide a lot of benefits. Additionally, there will still need to be a pre-processor involved to deduplicate and minifiy the css code.
This ADR is not currently too concerned about the build time for paragon or common theme. The idea is to make it possible to inject variables after the theme is built so that a lot of common theming usecases can be accomplished without recompiling dozens of MFEs.
This is a first step I see. Going from needing to rebuild 12 MFEs to rebuilding one theme. The next step can then involve reducing how much of that theme needs to be rebuilt till we reach a point where a single css file having all the necessary variables is all that's needed, and can be even generated dynamically.
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.
(apologies in advance for the long response 😅)
Yes, CSS-in-JS has been largely dropped for now. That said, I was mostly asking just to get any alternatives considered briefly written as part of this ADR for context 😄
There isn't currently anywhere currently where this is written down, except for these notes from a recent Paragon Working Group meeting about design tokens (raw notes from the discussion). I'm hoping to draft an ADR around
style-dictionary
and propose it to the Frontend Working group in the near future with more details and the rationale.The purpose of a design system is exactly that, to maintain consistent style and patterns across platforms for a unified brand user experience 😄 That doesn't mean it can't be theme-able as well.
Design tokens, as intended, represent the fundamental atomic building blocks of a design system (e.g., spacing, typography, color, etc.). Today, our design tokens are largely implemented as SCSS variables. The W3C design token community group spec describes the context in a bit more detail.
Largely, the proposal for
style-dictionary
is not define our design tokens as either CSS variables or SCSS variables, but to instead define them as a standard format for a design token in JSON. By usingstyle-dictionary
we could automatically generate both CSS and SCSS variables representing the same design token values.style-dictionary
could also generate the same design token values as CSS utility classes for us in a standardized way, too.style-dictionary
is largely for defining design tokens as a standard format, so that it can be transformed/exported for consumption by a variety of platforms.style-dictionary
can also export to a data format that Sketch, and possibly even Figma, can use which is intriguing for collaboration with designs and having truly a single source of truth for our design tokens.Exactly 😄 From a build time perspective,
style-dictionary
should only be an issue for Paragon and the common theme itself, but not consumers. The impactstyle-dictionary
may have on consumers is a breaking change in some way such that the SCSS variable names change from what they are now;style-dictionary
is just responsible for creating which CSS variables, SCSS variables, and CSS utility classes are available to use (among other potential export formats for mobile apps).Then, in the Paragon components they would likely start reading from the CSS variables instead of SCSS variables, which can be themed at run-time and is the goal of this current ADR. In other words,
style-dictionary
(and how we configure it) would be responsible generating the files and output for the public API for theming the Paragon design system, and potentially enable theming with both SCSS and CSS variables depending on the consumers needs is the idea.At a high level, we would like to make Paragon more platform-agnostic, while maintaining its theming capabilities (e.g., perhaps a web component implementation of the design system component library could be interesting).
So, not entirely sure yet, but I believe
style-dictionary
would potentially impact MFEs using:style-dictionary
. As an example, our color design tokens might change to a standard format of$pgn-color-warning-900
vs.$warning-900
now (or--pgn-color-warning-900
vs.--warning-900
), which with well-documented release notes / migration guide / CLI tool shouldn't be too bad of an upgrade?style-dictionary
per our configuration (e.g.,.pgn-color-warning-900
).style-dictionary
and working on a proof-of-concept prototype.If you want to try it locally, checkout that linked branch, run
npm run install-all
, and thennpm run build_tokens
. You'll see it generate a few files from the source design tokens defined as JSON objects in the./tokens
directory:style-dictionary-build/scss/_variables.scss
style-dictionary-build/css/variables.css
style-dictionary-build/js/variables.js
It'd impact any custom themes (e.g.,
@edx/brand-edx.org
) as they, too, would likely need to migrate to the new standard variables instead of the current ones as well.We are in the early stages of thinking through the implications of
style-dictionary
, but I do think it's very much aligned with the goal of this ADR. I believestyle-dictionary
is largely seen as a way to implement the CSS variables proposed by this ADR in a reasonably scalable, and platform-agnostic way.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.
Another reason I'd love for Paragon to move away from Bootstrap nomenclature is because consumers often rely on the internals of Bootstrap (classes, mixins, variables, etc), which in theory should be just an implementation detail of the Paragon library, not really exposed to the consumer.
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.
The link isn't redirect because the link reference is
Bootstrap 5.x
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.
I think is a good idea to find a runtime theming option and this ADR is a good starting point and could show the capabilities of CSS variables for the open edx ecosystem. I support 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.
I think it'd be great to strive to theming capabilities with either CSS variables or SCSS variables depending on the needs of the consumer/application. Is the intention here to no longer rely on SCSS variables all, and if so, it's worth calling out the impacts of that on consumers in terms of necessary refactors, etc. We'd want to ensure we minimize breaking changes as much as possible.
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.
The intention isn't to abandon SCSS variables, those will still be there, and before style-tokens are realised, they will probably be the source of the CSS variable values. So in cases where SCSS variables are directly needed, they can be used.
However, what it's building towards is that MFEs should stop using SCSS variables, since that will allow them to be built independently of Paragon, so that theme changes can be applied to an MFE without rebuilding it. Details on that are coming in a separate ADR soon.