Remove `sass-vars-to-js-loader` dependency by cee-chen · Pull Request #6444 · elastic/eui

cee-chen · 2022-11-30T00:22:52Z

Summary

This PR is part of a higher-level goal of removing our node-sass dependency (see #6442).

This PR handles 2 remaining instances in our src-docs/ calling sass-vars-to-js-loader by either importing the JSON variables directly or using the useJsonVars util added in #5227.

The biggest actual change (which I will highlight several areas of uncertainty) was sass-vars-to-js-loader being called in our exported Elastic Charts themes. As this affects actual source code vs docs only, it is by far the highest risk (although in theory it should work as before).

I strongly recommend code-reviewing by commit.

QA

Go to https://eui.elastic.co/pr_6444/#/theming/typography/values#font-weight?themeLanguage=sass
- Confirm the "I am proper bold" is still bold and has the same font-weight as production
- Confirm the "Variable fonts" range slider still looks and works as production does
Go to https://eui.elastic.co/pr_6444/#/elastic-charts/creating-charts#theming-via-eui
- Confirm all charts on the page have the same color/appearance as production

General checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
~~- [ ] A changelog entry exists and is marked appropriately~~ Open to thoughts, but since this in theory should not impact either end users or consumers, I skipped the changelog

- copies existing `useJsonVars` usage in file

- import JSON file directly of `useJsonVars` since we're just grabbing font weight vars and theme color is irrelevant

- this will have to be done anyway if/when we completely remove Sass etc., so it seems prescient to do now - nothing end-user-facing should have changed as a result of this refactor

…s vars - some slightly opinionated changes in here - I've left TODOs on things I'm unsure about, will also elaborate more in github comments

cee-chen · 2022-11-30T00:35:15Z

src/themes/charts/themes.ts

+// Build a static output of the EUI Amsterdam theme colors
+// TODO: At some point, should EuiCharts be able to inherit or create a theme dynamically from our theme provider?
+const KEY = '_EUI_CHART_THEME_AMSTERDAM';
+const builtTheme = buildTheme({}, KEY) as typeof EuiThemeAmsterdam;
+const lightColors = getComputed(EuiThemeAmsterdam, builtTheme, 'LIGHT').colors;
+const darkColors = getComputed(EuiThemeAmsterdam, builtTheme, 'DARK').colors;


A couple points here:

This is basically me statically calling what our EuiThemeProvider dynamically does:

eui/src/services/theme/provider.tsx

Lines 115 to 119 in 4932c51

getComputed(

system,

buildTheme(modifications, `_${system.key}`) as typeof system,

colorMode

)

The major downside to this (that already existed with the Sass variables, to be clear) is that it does not respond automatically to colorMode or dynamically update to consumer on-the-fly modifications and so forth.
I do think we need to reconsider how we export EUI_CHARTS_THEME_LIGHT and EUI_CHARTS_THEME_DARK. If we want to make our charts theme dynamic and future-proof, we may want to change it from static imports to a React hook or similar instead.

I will say however there is a benefit to making all our theme variables available as a static, no-modifications obj... For example, for dev-only testing, or for when Sass is removed and if we want to programmatically build a .scss file of all our various theme vars - we will need this statically generated built theme at that point. I honestly considered DRYing out this logic and putting it in amsterdam/theme.ts, but wasn't sure if we needed that just yet.

cee-chen · 2022-11-30T00:36:47Z

src/themes/charts/themes.ts

+  // TODO: Should theme colors be part of the base EuiTheme, or separate?
+  chartLines: shade(lightColors.lightestShade, 0.03),
+  chartBand: lightColors.lightestShade,


These variables did not exist in any of our current JS vars but they appear to exist in Sass:

eui/src/themes/amsterdam/_colors_light.scss

Lines 45 to 47 in c88dd35

// Charts

$euiColorChartLines: shade($euiColorLightestShade, 3%) !default;

$euiColorChartBand: $euiColorLightestShade !default;

@daveyholler @miukimiu do either of you have opinions on whether we need to make this variable live in our current _colors.ts, or is this a very charts-specific color that we do not want to make globally available?

Based off naming, I'm assuming that this is very charts-specific. I don't see a need for them to be globally available.

Works for me - thanks Davey. I'll go ahead and remove the TODO comments, and we can always revisit later if we decide to make them more available.

It is maybe worth noting that there are 2 instances in Kibana of developers using euiColorChartLines exported from our JSON files: https://github.com/search?q=repo%3Aelastic%2Fkibana%20euiColorChartLines&type=code

To be honest, I'm not totally sure how to reconcile that usage if and when we get rid of our Sass vars and JSON files. Those 2 usages may have to import EUI_CHARTS_THEME_LIGHT/DARK directly and dig through the theme to find the color they want.

cee-chen · 2022-11-30T00:36:50Z

src/themes/charts/themes.ts

+  // TODO: Should these colors be part of the base EuiTheme, or separate?
+  chartLines: darkColors.lightShade,
+  chartBand: tint(darkColors.lightestShade, 0.025),


See above comment.

eui/src/themes/amsterdam/_colors_dark.scss

Lines 45 to 47 in cffdce0

// Charts

$euiColorChartLines: $euiColorLightShade !default;

$euiColorChartBand: tint($euiColorLightestShade, 2.5%) !default;

kibanamachine · 2022-11-30T00:50:28Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6444/

- interesting that it only started throwing after recent `.json.d.ts` additions - ah well

kibanamachine · 2022-11-30T20:27:27Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6444/

daveyholler

I've pulled this down and tested locally. I've also spot checked with the QA checklist in the PR description. I didn't find any issues with it.

- remove TODOs per designer feedback

kibanamachine · 2022-12-01T20:03:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6444/

breehall

👍 This looks good to me. I pulled the branch to test and used the PR preview to confirm validation criteria.

cee-chen added 5 commits November 29, 2022 15:07

Remove usage of sass-vars-to-js-loader to grab font weight

aad8fe2

- copies existing `useJsonVars` usage in file

Remove usage of sass-vars-to-js-loader for font weights

cf5489e

- import JSON file directly of `useJsonVars` since we're just grabbing font weight vars and theme color is irrelevant

[optional] Refactor variable font range to use JS vars, not Sass

bf79364

- this will have to be done anyway if/when we completely remove Sass etc., so it seems prescient to do now - nothing end-user-facing should have changed as a result of this refactor

Convert EUI charts themes to use JS theme vars instead of loading Sas…

3e20684

…s vars - some slightly opinionated changes in here - I've left TODOs on things I'm unsure about, will also elaborate more in github comments

Remove sass-vars-to-js-loader dependency

778afa4

cee-chen added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Nov 30, 2022

cee-chen mentioned this pull request Nov 30, 2022

[Meta] Fix developer experience for Apple M1/arm64 developers #6442

Closed

5 tasks

cee-chen marked this pull request as ready for review November 30, 2022 00:27

cee-chen commented Nov 30, 2022

View reviewed changes

cee-chen requested review from breehall and daveyholler November 30, 2022 16:49

cee-chen added 2 commits November 30, 2022 11:41

Merge branch 'main' into remove-sass-vars-to-js-loader

21e61fd

Ignore TS-error after merge

16da162

- interesting that it only started throwing after recent `.json.d.ts` additions - ah well

daveyholler approved these changes Dec 1, 2022

View reviewed changes

Comments

5289581

- remove TODOs per designer feedback

breehall approved these changes Dec 2, 2022

View reviewed changes

cee-chen merged commit fc30ca8 into elastic:main Dec 2, 2022

cee-chen deleted the remove-sass-vars-to-js-loader branch December 2, 2022 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `sass-vars-to-js-loader` dependency#6444

Remove `sass-vars-to-js-loader` dependency#6444
cee-chen merged 8 commits intoelastic:mainfrom
cee-chen:remove-sass-vars-to-js-loader

cee-chen commented Nov 30, 2022 •

edited

Loading

Uh oh!

cee-chen Nov 30, 2022

Uh oh!

cee-chen Nov 30, 2022

Uh oh!

daveyholler Dec 1, 2022

Uh oh!

cee-chen Dec 1, 2022

Uh oh!

cee-chen Nov 30, 2022

Uh oh!

kibanamachine commented Nov 30, 2022

Uh oh!

kibanamachine commented Nov 30, 2022

Uh oh!

daveyholler left a comment

Uh oh!

kibanamachine commented Dec 1, 2022

Uh oh!

breehall left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	getComputed(
	system,
	buildTheme(modifications, `_${system.key}`) as typeof system,
	colorMode
	)

	// Charts
	$euiColorChartLines: shade($euiColorLightestShade, 3%) !default;
	$euiColorChartBand: $euiColorLightestShade !default;

	// Charts
	$euiColorChartLines: $euiColorLightShade !default;
	$euiColorChartBand: tint($euiColorLightestShade, 2.5%) !default;

Conversation

cee-chen commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

cee-chen Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

daveyholler Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 30, 2022

Uh oh!

kibanamachine commented Nov 30, 2022

Uh oh!

daveyholler left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Dec 1, 2022

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Nov 30, 2022 •

edited

Loading