Skip to content
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

Closed

Conversation

xitij2000
Copy link
Contributor

@xitij2000 xitij2000 commented Jun 16, 2022

Description

Adds an ADR for a plan to expose all Bootstrap variables used by Paragon and MFEs as CSS variables.

Deploy Preview

NA

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

Adds an ADR for a plan to expose all Bootstrap variables used by Paragon and MFEs
as CSS variables.
@openedx-webhooks
Copy link

Thanks for the pull request, @xitij2000! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 16, 2022
@netlify
Copy link

netlify bot commented Jun 16, 2022

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d711d2e
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/634668c832627f0009181b9f
😎 Deploy Preview https://deploy-preview-1388--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Base: 90.97% // Head: 90.36% // Decreases project coverage by -0.61% ⚠️

Coverage data is based on head (d711d2e) compared to base (45887c9).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1388      +/-   ##
==========================================
- Coverage   90.97%   90.36%   -0.62%     
==========================================
  Files         195      209      +14     
  Lines        3170     3581     +411     
  Branches      715      838     +123     
==========================================
+ Hits         2884     3236     +352     
- Misses        273      330      +57     
- Partials       13       15       +2     
Impacted Files Coverage Δ
src/Tabs/Tab.jsx 50.00% <0.00%> (-50.00%) ⬇️
src/Nav/index.jsx 50.00% <0.00%> (-50.00%) ⬇️
src/Navbar/index.jsx 52.17% <0.00%> (-32.04%) ⬇️
src/Modal/ModalLayer.jsx 68.42% <0.00%> (-31.58%) ⬇️
src/Button/index.jsx 73.33% <0.00%> (-26.67%) ⬇️
src/Popover/index.jsx 75.00% <0.00%> (-25.00%) ⬇️
src/Overlay/index.jsx 81.81% <0.00%> (-18.19%) ⬇️
src/Modal/AlertModal.jsx 33.33% <0.00%> (-16.67%) ⬇️
src/Modal/StandardModal.jsx 50.00% <0.00%> (-16.67%) ⬇️
src/Modal/FullscreenModal.jsx 50.00% <0.00%> (-16.67%) ⬇️
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adamstankiewicz adamstankiewicz self-requested a review June 17, 2022 10:26
docs/decisions/0018-expose-all-sass-vars-as-css-vars.rst Outdated Show resolved Hide resolved
docs/decisions/0018-expose-all-sass-vars-as-css-vars.rst Outdated Show resolved Hide resolved
Paragon + Bootstrap since in that case any changes to the core theme will
require a rebuild of each MFE.

Decision
Copy link
Member

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 then style-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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamstankiewicz

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 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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

@adamstankiewicz adamstankiewicz Jun 24, 2022

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 😅)

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?

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.

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 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 using style-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.

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 use cases can be accomplished without recompiling dozens of MFEs.

Exactly 😄 From a build time perspective, style-dictionary should only be an issue for Paragon and the common theme itself, but not consumers. The impact style-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:

  • SCSS variables. they may/will change to a new standard format, generated by 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?
  • CSS utility classes. those too might change for a standard format, output by style-dictionary per our configuration (e.g., .pgn-color-warning-900).
  • I'm likely missing something 😄 Again, still some investigation to do through writing up an ADR about 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 then npm 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 believe style-dictionary is largely seen as a way to implement the CSS variables proposed by this ADR in a reasonably scalable, and platform-agnostic way.

docs/decisions/0018-expose-all-sass-vars-as-css-vars.rst Outdated Show resolved Hide resolved
automatically apply all the latest variables.

With this, the common branding theme becomes a runtime dependency rather than a
build time dependency, allowing it to be replaced at run time without needing to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[clarification] How does this approach to run time theming differ to run time theming with CSS-in-JS, with a theme object that you pass to a ThemeProvider type component? Are there advantages/disadvantages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the biggest advantage of using CSS variables vs CSS-in-JS is that it can work with the existing code with few changes. We can just do a search for "$primary-500" and replace it with "var(--pgn-primary-500)" and it will work as expected. Even CSS-in-JS can continue working with CSS variables AFAIK.

Consequences
************

Wherever we are currently using SASS variables, we can now use CSS variables.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[clarification] Are these consequences for the consumers of Paragon (e.g., MFEs), or for the component library itself? Or both? It might be worth separating out how each stakeholder might be impacted by CSS variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will do that.

But roughly, my idea was to leave Paragon's usage of SCSS intact, since it is a shared library, so it can be built and deployed once. I think all of Paragon's SCSS is included with each MFE anyway so right now it doesn't matter as much.

However, in the future if all of Paragon also uses CSS variables then it too can be built once, and ideally we boil down the compilation to just the variable definitions. The benefit of that will be much quicker compilation of the "theme", and potential near-real-time theme previewing.

Paragon + Bootstrap since in that case any changes to the core theme will
require a rebuild of each MFE.

Decision
Copy link
Member

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?

@natabene
Copy link

@xitij2000 Thank you for the contribution!

@xitij2000 xitij2000 changed the title docs: Add ADR for CSS variables [BB-6307] docs: ADR to expose all SCSS variables that MFEs use as CSS variables [BB-6307] Jun 24, 2022
Paragon + Bootstrap since in that case any changes to the core theme will
require a rebuild of each MFE.

Decision
Copy link
Member

@adamstankiewicz adamstankiewicz Jun 24, 2022

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 😅)

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?

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.

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 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 using style-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.

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 use cases can be accomplished without recompiling dozens of MFEs.

Exactly 😄 From a build time perspective, style-dictionary should only be an issue for Paragon and the common theme itself, but not consumers. The impact style-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:

  • SCSS variables. they may/will change to a new standard format, generated by 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?
  • CSS utility classes. those too might change for a standard format, output by style-dictionary per our configuration (e.g., .pgn-color-warning-900).
  • I'm likely missing something 😄 Again, still some investigation to do through writing up an ADR about 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 then npm 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 believe style-dictionary is largely seen as a way to implement the CSS variables proposed by this ADR in a reasonably scalable, and platform-agnostic way.

Paragon can create CSS variable exports like: `--pgn-blue`, `--pgn-primary`,
`--pgn-primary-200`` etc. The reason for prefixing with `--pgn` is to isolate
changes to these names in Bootstrap. For example, in Bootstrap 4.x these are
named as `--blue, --primary` etc. However in `Bootstrap 5`_, they are prefixed
Copy link
Member

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.

docs/decisions/0018-expose-all-sass-vars-as-css-vars.rst Outdated Show resolved Hide resolved
************

Once this is implemented, MFEs, and other consumers of Pargaon can start using
CSS variables, wherever they are currently using SASS variables. The
Copy link
Member

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.

Copy link
Contributor Author

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.

Paragon can create CSS variable exports like: `--pgn-blue`, `--pgn-primary`,
`--pgn-primary-200`` etc. The reason for prefixing with `--pgn` is to isolate
changes to these names in Bootstrap. For example, in Bootstrap 4.x these are
named as `--blue, --primary` etc. However in `Bootstrap 5`_, they are prefixed
Copy link
Contributor

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

Copy link
Contributor

@dcoa dcoa Jul 21, 2022

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.

@xitij2000
Copy link
Contributor Author

xitij2000 commented Sep 12, 2022

I've created a starting implementation draft PR for this: #1620

xitij2000 and others added 4 commits October 12, 2022 07:11
Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com>
Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com>
Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com>
Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com>
@openedx-webhooks
Copy link

@xitij2000 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Discovery: Reconcile Opencraft's work on MFE Theming with 2U initiatives
7 participants