Convert sass variables to css variables #35385

farhan · 2024-08-28T07:12:26Z

Relevant PR's

Predecessor PR: #35233
Next ticket: #35300
Word Cloud & Annotatable XBlocks have already been merged in following PR's
These PR's were merged earlier to check the impact of early sass to css variables migration on the production.
Word Cloud XBlock: #35386
Annotatable XBlock: #35409

Dropping use of sass/css variable:

We will drop use of $all-text-inputs in the file xmodule/assets/capa/_display.scss at line ~1075

  #{$all-text-inputs} {
    display: inline;
    width: auto;
  }

It will be compiled into following css in ticket and only that css will be used in future

  .xmodule_display.xmodule_ProblemBlock div.problem input[type="email"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="number"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="password"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="search"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="tel"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="text"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="url"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="color"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="date"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="datetime"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="datetime-local"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="month"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="time"], .xmodule_display.xmodule_ProblemBlock div.problem input[type="week"] {
    display: inline;
    width: auto; 
}

Compiled Sass, picking values from global css variables:

:root {
  --action-primary-active-bg: #0075b4;
  --base-font-size: 1em;
  --base-line-height: 1.5em;
  --baseline: 20px;
  --black: #000;
  --black-t2: rgba(0, 0, 0, 0.5);
  --blue: #0075b4;
  --blue-d1: #005e90;
  --blue-d2: #00466c;
  --blue-d4: #001724;
  --blue-s1: #0075b4;
  --body-color: #313131;
  --border-color: #e7e7e7;
  --bp-screen-lg: 1024px;
  --btn-brand-focus-background: #065683;
  --correct: #008100;
  --danger: #b20610;
  --darkGrey: #8891a1;
  --error-color: #cb0712;
  --error-color-dark: #95050d;
  --error-color-light: #f95861;
  --font-bold: 700;
  --font-family-sans-serif: "Open Sans", "Helvetica Neue", Helvetica, Arial, sans-serif;
  --general-color-accent: #0075b4;
  --gray: #767676;
  --gray-300: #d9d9d9;
  --gray-d1: #5e5e5e;
  --gray-l2: #adadad;
  --gray-l3: #c8c8c8;
  --gray-l4: #e4e4e4;
  --gray-l6: #f8f8f8;
  --icon-correct: url("../images/correct-icon.png");
  --icon-incorrect: url("../images/incorrect-icon.png");
  --icon-info: url("../images/info-icon.png");
  --icon-partially-correct: url("../images/partially-correct-icon.png");
  --icon-spinner: url("../images/spinner.gif");
  --icon-unanswered: url("../images/unanswered-icon.png");
  --incorrect: #b20610;
  --lightGrey: #edf1f5;
  --lighter-base-font-color: #646464;
  --link-color: #1b6d99;
  --medium-font-size: 0.9em;
  --partially-correct: #008100;
  --primary: #0075b4;
  --shadow: rgba(0, 0, 0, 0.2);
  --shadow-l1: rgba(0, 0, 0, 0.1);
  --sidebar-color: #f6f6f6;
  --small-font-size: 80%;
  --static-path: "..";
  --submitted: #0075b4;
  --success: #008100;
  --tmg-f2: 0.25s;
  --tmg-s2: 2s;
  --transparent: transparent;
  --uxpl-gray-background: #d9d9d9;
  --uxpl-gray-base: #414141;
  --uxpl-gray-dark: #111111;
  --very-light-text: white;
  --warning: #e2c01f;
  --warning-color: #ffc01f;
  --warning-color-accent: #fffcdd;
  --white: #fff;
  --yellow: #e2c01f; 
  }

Possible ripples (God forbid) PR merging could produce:

Riples could be on the frontend styling of edx-platform Build in XBlocks. XBlocks list could be find here

kdmccormick · 2024-08-28T15:51:26Z

@farhan ty for writing up those issues.

Regarding issue 1, seems that there is no lighten/darken replacement in CSS. There is a separate CSS brightness filter that could be applied to the same elements, but it might be difficult to get an exact match with Sass's lighten and darken.

Regarding issue 2, from my research it seems that string concatenation in CSS just simply isn't supported.

Perhaps an easy for for both issues 1 and 2 would be to define new CSS variables in _builtin-block-variables.scss :

:root {
  ...
  --error-color-dark: darken($error-color, 11%);
  --error-color-light: lighten($error-color, 25%);
  --unanswered-icon-url: '#{var(--static-path)}/images/unanswered-icon.png';
  ...
}

how does that look?

farhan · 2024-08-29T10:24:53Z

@kdmccormick
Solutions sounds reasonable to me.
was inclined towards brightness filter but for the exact match I think let's go for it.

@kdmccormick Followup question:
Are we planning to remove all the sass infrastructure?
If so, will these sass variables will also be removed ahead?

farhan · 2024-09-04T10:54:37Z

@kdmccormick Another issue is identified, details added in description:
Can we do hard code just for all-text-inputs, its using in the Cappa Problem XBlock?

kdmccormick · 2024-09-04T13:45:50Z

@farhan

Are we planning to remove all the sass infrastructure?
If so, will these sass variables will also be removed ahead?

The general edx-platform Sass infrastructure must remain until the very last legacy frontend is converted into a React micro-frontend. That will not be for another year or two. The Sass variables we use in _builtin-block-variables.scss will remain available for the time being.

However, we will stop compiling built-in XBlock Sass in particular as part of this ticket: #35300

kdmccormick · 2024-09-04T13:54:54Z

@farhan

Can we do hard code just for all-text-inputs, its using in the Cappa Problem XBlock?

Yes, you can hard code the value of $all-text-inputs and remove it from _builtin-block-variables.scss. I cannot imagine that anyone's theme would have modified the value of that variable, since it really is just an exhaustive list of all text-based input types.

kdmccormick

The code is looking good! Once you apply the all-input-types change you've proposed @farhan , I'll give it a manual test, and then we should be good to ship this.

open-craft-grove · 2024-09-04T15:34:52Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-09-05T06:18:36Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

farhan · 2024-09-05T06:33:10Z

@kdmccormick PR is rebased and ready to ship from my side.

FYI We have already shipped Annotatable XBlock in PR tested by me and @irtazaakram .

open-craft-grove · 2024-09-05T06:48:02Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-09-05T07:17:46Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-09-10T08:09:51Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-09-10T11:03:21Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

kdmccormick

I was able to confirm that this works. For instance, here is a site running a tutor-indigo theme where the primary color is set to pink:

What is happening is:

we install tutor-indigo with INDIGO_PRIMARY_COLOR=#dd00dd (pink):
tutor-indigo sets $primary: {{ INDIGO_PRIMARY_COLOR }}
edx-platform's base sass sets $general-color-accent: theme-color("primary") !default;
edx-platform's _builtin-block-variables.scss module sets --general-accent-color: $general-accent-color
the ProblemBlock answer notification widget references var(--general-accent-color), which, as expected, turns out pink.

This demonstrates to me that the style variables are all being passed down from Sass to CSS as we want them to 👍🏻 Great work @farhan !

kdmccormick · 2024-09-11T18:57:42Z

One very minor note @farhan , I will be squashing your commit with a refactor: message instead of a chore: message. Chores refer to insignificant changes that can mostly be ignored, whereas this is a fairly significant refactoring that we had to test carefully.

edx-pipeline-bot · 2024-09-11T20:18:06Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-09-11T20:35:24Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-09-11T20:35:25Z

2U Release Notice: This PR has been deployed to the edX production environment.

nsprenkle · 2024-09-12T15:48:27Z

xmodule/assets/video/_display.scss


 margin-top: -0.6em;
 font-family: 'FontAwesome';
 content: "\f142";
- color: $white;
+ color: var(--white);


This line resolves incorrectly in the built CSS:

color: var(--#fff)

Instead of

color: var(--white); // or color: #fff;

farhan force-pushed the farhan/sass-to-css-variables branch 4 times, most recently from 87063d8 to 0c7d786 Compare August 28, 2024 07:40

farhan mentioned this pull request Aug 28, 2024

chore: Convert word cloud block sass variables to css variables #35386

Merged

farhan requested review from irtazaakram and kdmccormick August 28, 2024 09:48

farhan force-pushed the farhan/sass-to-css-variables branch 2 times, most recently from 632b6bf to 63a6b9c Compare August 28, 2024 10:16

farhan changed the title ~~Farhan/sass to css variables~~ Convert sass to css variables Aug 28, 2024

farhan force-pushed the farhan/sass-to-css-variables branch 2 times, most recently from 252fd10 to ea08309 Compare August 30, 2024 07:08

farhan mentioned this pull request Sep 2, 2024

Convert annotatable block sass variables to css variables #35409

Merged

farhan force-pushed the farhan/sass-to-css-variables branch from ea08309 to 3fdb217 Compare September 2, 2024 12:37

farhan marked this pull request as ready for review September 2, 2024 12:38

farhan closed this Sep 2, 2024

farhan reopened this Sep 2, 2024

kdmccormick added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Sep 4, 2024

kdmccormick reviewed Sep 4, 2024

View reviewed changes

farhan force-pushed the farhan/sass-to-css-variables branch 4 times, most recently from 64a78c5 to e35f3a5 Compare September 5, 2024 06:03

chore: Convert builtin blocks sass variables to css variables

e666b81

farhan force-pushed the farhan/sass-to-css-variables branch from e35f3a5 to e666b81 Compare September 5, 2024 06:11

farhan changed the title ~~Convert sass to css variables~~ Convert sass variables to css variables Sep 6, 2024

Merge branch 'master' into farhan/sass-to-css-variables

5fa5ad5

farhan requested a review from kdmccormick September 10, 2024 07:18

Merge branch 'master' into farhan/sass-to-css-variables

d79f929

kdmccormick approved these changes Sep 11, 2024

View reviewed changes

kdmccormick merged commit 082350e into master Sep 11, 2024
49 checks passed

kdmccormick deleted the farhan/sass-to-css-variables branch September 11, 2024 18:59

nsprenkle reviewed Sep 12, 2024

View reviewed changes

nsprenkle mentioned this pull request Sep 12, 2024

revert: "Convert sass variables to css variables" #35480

Merged

farhan mentioned this pull request Sep 16, 2024

chore: Converts builtin block Sass variables into CSS variables #35369

Closed

farhan linked an issue Sep 16, 2024 that may be closed by this pull request

Convert builtin block Sass variables into CSS variables #35306

Closed

ormsbee mentioned this pull request Sep 27, 2024

XBlock asset support for new libraries experience (WIP) #35557

Draft

farhan restored the farhan/sass-to-css-variables branch October 1, 2024 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert sass variables to css variables #35385

Convert sass variables to css variables #35385

farhan commented Aug 28, 2024 •

edited

Loading

kdmccormick commented Aug 28, 2024

farhan commented Aug 29, 2024 •

edited

Loading

farhan commented Sep 4, 2024 •

edited

Loading

kdmccormick commented Sep 4, 2024

kdmccormick commented Sep 4, 2024

kdmccormick left a comment

open-craft-grove commented Sep 4, 2024

open-craft-grove commented Sep 5, 2024

farhan commented Sep 5, 2024 •

edited

Loading

open-craft-grove commented Sep 5, 2024

open-craft-grove commented Sep 5, 2024

open-craft-grove commented Sep 10, 2024

open-craft-grove commented Sep 10, 2024

kdmccormick left a comment

kdmccormick commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

nsprenkle Sep 12, 2024

Convert sass variables to css variables #35385

Convert sass variables to css variables #35385

Conversation

farhan commented Aug 28, 2024 • edited Loading

Relevant PR's

Dropping use of sass/css variable:

Compiled Sass, picking values from global css variables:

Possible ripples (God forbid) PR merging could produce:

kdmccormick commented Aug 28, 2024

farhan commented Aug 29, 2024 • edited Loading

farhan commented Sep 4, 2024 • edited Loading

kdmccormick commented Sep 4, 2024

kdmccormick commented Sep 4, 2024

kdmccormick left a comment

Choose a reason for hiding this comment

open-craft-grove commented Sep 4, 2024

open-craft-grove commented Sep 5, 2024

farhan commented Sep 5, 2024 • edited Loading

open-craft-grove commented Sep 5, 2024

open-craft-grove commented Sep 5, 2024

open-craft-grove commented Sep 10, 2024

open-craft-grove commented Sep 10, 2024

kdmccormick left a comment

Choose a reason for hiding this comment

kdmccormick commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

nsprenkle Sep 12, 2024

Choose a reason for hiding this comment

farhan commented Aug 28, 2024 •

edited

Loading

farhan commented Aug 29, 2024 •

edited

Loading

farhan commented Sep 4, 2024 •

edited

Loading

farhan commented Sep 5, 2024 •

edited

Loading