Base Styles: Import colors into variables

[ockham]

Description

@import "./colors"; from within _variables.scss, since the latter depends on $dark-gray-900 which is defined in the former.

Found while working on Automattic/wp-calypso#38445.

How has this been tested?

Try importing just @wordpress/base-styles/variables in your app, using the currently released package. You'll get a build error saying something like

    Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
    SassError: Undefined variable: "$dark-gray-900".

Now try with the changes that this PR makes. Your style files should build successfully 👍

Types of changes

Bug fix.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[simison]

Hmm right, I suppose elsewhere we've always been importing all the files at once so that all variables always exist. I.e. as highlighted in docs:

https://github.com/WordPress/gutenberg/tree/bfd1117846c7802d62dbba046c27968d961781e7/packages/base-styles#scss-utilities-and-variables

Things obviously won't work when importing just one file directly like in your PR.

The build/test step during Gutenberg-CI runs won't catch these issues either — should it?

[ockham]

CircleCI's Lint job is failing, complaining that it

 Expected double quotes

Which I find a bit surprising? Is Gutenberg using double quotes in SASS imports?

[ockham]

Hmm right, I suppose elsewhere we've always been importing all the files at once so that all variables always exist. I.e. as highlighted in docs:

https://github.com/WordPress/gutenberg/tree/bfd1117846c7802d62dbba046c27968d961781e7/packages/base-styles#scss-utilities-and-variables

Things obviously won't work when importing just one file directly like in your PR.

The build/test step during CI runs won't catch these either — should it?

I think that'd make sense 👍

[simison]

lint error [...] Which I find a bit surprising? Is Gutenberg using double quotes in SASS imports?

Yeah — Gutenberg prolly uses https://www.npmjs.com/package/stylelint-config-wordpress which is double quotes indeed, if I remember correctly.

[ockham]

lint error [...] Which I find a bit surprising? Is Gutenberg using double quotes in SASS imports?

Yeah — Gutenberg prolly https://www.npmjs.com/package/stylelint-config-wordpress which is double quotes indeed, if I remember correct.

Pushed a fix 👍

[simison]

The build/test step during CI runs won't catch these either — should it?

I think that'd make sense 👍

I personally think we should render each file separately as a test run and ensure;

• ensure they don't fatal
• they don't produce any visible CSS (we concluded not to test for this in the previous convo at Add base-styles package #17883)

More convo about testing in original PR #17883 — see commits for details and an implementation example. I don't think that needs to be done in this PR tho but prolly easy to catch (and fix) any other missing imports that way.

[gziolo]

It would be nice to include a section title in the changelog to make it easier to read. Thanks for the fix 💯

[hypest]

It seems that the native mobile side is broken by this change, with this error:

Error: ./colors could not be resolved in /home/circleci/project/gutenberg/packages/edit-post/src/components/layout,/home/circleci/project,gutenberg/packages/edit-post/src/components/layout,/home/circleci/project/src,/home/circleci/project/gutenberg/assets/stylesheets,/home/circleci/project/gutenberg/packages/base-styles

My first impression is that the SASS transformer we use in the RN app pipeline doesn't understand partial SASS files, and is unable to resolve the (existing) ./_colors.scss.

[simison]

@hypest is the solution to import ./_colors.scss (i.e. include the file extension) in Gutenberg or change something at your side?

[hypest]

👋 @simison!

On the Gutenberg-web side, including the underscore is fixing it for native. The native mobile SASS transformer apparently doesn't support partial SASS files so, it doesn't automatically try to resolve the underscore-less file to the underscored one.

If handled on the native mobile side alone, a solution seems to be to introduce a new file colors.native.scss, and in it just @import "./_colors". It's a bit hacky but, can be a workaround.

I wonder though, perhaps it's an obsolete impression but, I thought that the colors.scss files were automatically imported for all SASS files so, one didn't need to explicitly import them 🤔. That's what we had implemented in native mobile too, automatically import _colors.scss and others.

[ockham]

I wonder though, perhaps it's an obsolete impression but, I thought that the colors.scss files were automatically imported for all SASS files so, one didn't need to explicitly import them . That's what we had implemented in native mobile too, automatically import _colors.scss and others.

The point of this PR was to enable 3rd party projects to import individual files from @wordpress/base-styles (specifically, _variables.scss) by making the -- otherwise implicit -- styling dependency chain explicit. Without the colors import, just importing _variables.scss wouldn't work, since the 3rd party project wouldn't know where to find the definition for $dark-gray-900, which is used in _variables.scss.

[ockham]

@hypest Are you sure about the partials support thing? I thought that the underscore/non-underscore aliasing was built into SASS at a pretty low level (but I might be wrong).

[ockham]

It seems that the native mobile side is broken by this change, with this error:

Error: ./colors could not be resolved in /home/circleci/project/gutenberg/packages/edit-post/src/components/layout,/home/circleci/project,gutenberg/packages/edit-post/src/components/layout,/home/circleci/project/src,/home/circleci/project/gutenberg/assets/stylesheets,/home/circleci/project/gutenberg/packages/base-styles

My first impression is that the SASS transformer we use in the RN app pipeline doesn't understand partial SASS files, and is unable to resolve the (existing) ./_colors.scss.

@hypest Do you have some more context on this (e.g. failing CI job link)? 🙂
It's kinda weird that it's not resolving the file correctly...

[ockham]

Ah I think I found the relevant file https://github.com/wordpress-mobile/gutenberg-mobile/blob/06b924d832e1bdd5bcbb2961f8c5e2051d772269/sass-transformer.js

[hypest]

@hypest Are you sure about the partials support thing? I thought that the underscore/non-underscore aliasing was built into SASS at a pretty low level (but I might be wrong).

I'm not too sure, I've only assuming that the SASS transformer we use in the RN mobile port (this library: https://github.com/kristerkari/react-native-sass-transformer) doesn't support it because if I just add the underscore the import is resolved OK.

@hypest Do you have some more context on this (e.g. failing CI job link)? 🙂
It's kinda weird that it's not resolving the file correctly...

The only CI indication that this fails is on the gutenberg-mobile repo for now. See this https://app.circleci.com/jobs/github/wordpress-mobile/gutenberg-mobile/20081 and search for "colors could not be resolved".

[ockham]

Looks like it's coming from gutenberg-mobile itself: https://github.com/wordpress-mobile/gutenberg-mobile/blob/06b924d832e1bdd5bcbb2961f8c5e2051d772269/sass-transformer.js#L139

Can we maybe just make that thing smarter? https://github.com/wordpress-mobile/gutenberg-mobile/blob/06b924d832e1bdd5bcbb2961f8c5e2051d772269/sass-transformer.js#L133

[ockham]

Looks like it's coming from gutenberg-mobile itself: https://github.com/wordpress-mobile/gutenberg-mobile/blob/06b924d832e1bdd5bcbb2961f8c5e2051d772269/sass-transformer.js#L139

(I guess that's our local fork of https://github.com/kristerkari/react-native-sass-transformer)

[hypest]

Can we maybe just make that thing smarter? https://github.com/wordpress-mobile/gutenberg-mobile/blob/06b924d832e1bdd5bcbb2961f8c5e2051d772269/sass-transformer.js#L133

Perhaps. It feels that'd be equivalent to trying to add support for partials 😬. Maybe we can live with just trying out the underscored filename.

(I guess that's our local fork of https://github.com/kristerkari/react-native-sass-transformer)

Indeed.

[ockham]

Can we maybe just make that thing smarter? https://github.com/wordpress-mobile/gutenberg-mobile/blob/06b924d832e1bdd5bcbb2961f8c5e2051d772269/sass-transformer.js#L133

Perhaps. It feels that'd be equivalent to trying to add support for partials 😬

Reading https://sass-lang.com/guide, I got the impression that there isn't that much to partials, other than a nomenclature to signal that they shouldn't be turned into CSS on their own (but used as imports elsewhere), and the leading underscore alias resolution 🤷‍♂️

Maybe we can live with just trying out the underscored filename.

Yeah, that might work as well.

(I guess that's our local fork of https://github.com/kristerkari/react-native-sass-transformer)

Indeed.

👍

[ockham]

Turns out upstream already has support for partials: https://github.com/kristerkari/react-native-sass-transformer/blob/52884dd59582856fa17e2b2e8ca9efc37d412387/index.js#L41