feat: extract css variables to js and css files #181

montezume · 2018-10-22T09:36:09Z

*** THIS CONSTITUTES A BREAKING CHANGE 😱 ***

In MC, the @imports will have to either be removed, or to be changed to the new singular css file.

Summary

This is (as far as I can tell), the simplest way to extract the materials vars to js and css without refactoring the materials folder and making a whole bunch of decisions with regards to the tokens.

Approach.

Create an index.css file in materials that imports all of the variables we want to expose.
Use postcss-custom-properties to extract those into a js file (materials/materials.js) and css file (materials/materials.css)
Format that file with prettier.
Refactor the materials folder and move all internal files to the internal directory.
Only expose media-queries.mod.css, materials.css and materials.js through npm
Amend the rollup and webpack configs to use the extracted variables file instead of the array of files.
Update readme

emmenko

Good idea, I like it! 👌

emmenko · 2018-10-22T19:14:13Z

scripts/extract-materials.js

+      fs.writeFileSync(
+        `${exportPath}.css`,
+        prettier.format(exportedCss, { ...prettierConfig, parser: 'scss' })
+      );


Nice!

Should we maybe inject a comment about those files being autogenerated?

montezume · 2018-10-23T07:38:06Z

@emmenko Good call! Also would make sense to update the README

tdeekens

Nice. I think @dferber90 should comment if this aligns with this plan to pass styles as an object to react-select.

tdeekens · 2018-10-23T07:56:13Z

materials/exports/vars.js

@@ -0,0 +1,113 @@
+module.exports = {
+  customProperties: {


Just curious: is this something we can set? Why is it "wrapped"?

@tdeekens this is the shape that postcss-custom-properties generates and expects for it's importFrom.

There's an example here as well

tdeekens · 2018-10-23T07:59:12Z

scripts/extract-materials.js

+      fs.writeFileSync(
+        `${exportPath}.css`,
+        prettier.format(exportedCss, { ...prettierConfig, parser: 'scss' })
+      );


Nice to format them but to be fair: I don't mind too much how an autogenerated file is formatted. :)

@tdeekens it's either this or to add it to the prettier ignore 😆

montezume · 2018-10-23T08:17:39Z

.storybook/webpack.config.js

@@ -123,7 +108,7 @@ module.exports = (storybookBaseConfig, configType) => {
              }),
              postcssCustomProperties({
                preserve: false,
-                importFrom: materialSources,
+                importFrom: 'materials/exports/vars.css',


I don't like these names but I'm not feeling very creative 😞

I would rename imports.js/css to custom-properties.js/css. That name seems more appropriate. "custom properties" is more or less a fancy name for "css variables". And that's what we're exposing here.

montezume · 2018-10-23T09:33:12Z

package.json

@@ -19,8 +19,9 @@
    "dist",
    "i18n",
    "images",
-    "materials/*.mod.css",
-    "materials/**/*.mod.css",
+    "materials/materials.css",


maybe should just call them index.css and index.js?

How about materials/imports.css?

I'm also fine with the index, I don't mind much as long as it's proper documented

dferber90

I made the changes I was about to request (and some more). These are merely suggestions and I'm okay with undoing them in case you don't agree.

The latest commit:

renames imports.css to custom-properties.css
renames imports.js to custom-properties.json
exports customProperties from @commercetools-frontend/ui-kit for easier use on the consumer side (only one import instead of two; no "reach-into-the-internals")
fixes the prebuild script
changes the custom-properties.css generation by only writing to disk once (I found this a bit easier to follow after changing the write to JSON anyways)
fixes the machine-depended generated path in the css-comment to be machine-independent

dferber90 · 2018-10-23T13:40:11Z

package.json

@@ -35,7 +36,7 @@
  ],
  "scripts": {
    "changelog": "conventional-changelog -p angular -u",
-    "prebuild": "node ./scripts/generate-icon-exports.js && node ./scripts/generate-base-colors.js && node ./scripts/generate-base-shadows.js && node ./scripts/generate-colors-for-story.js",
+    "prebuild": "node ./scripts/generate-icon-exports.js && node ./scripts/generate-base-colors.js && node ./scripts/generate-base-shadows.js && node ./scripts/generate-colors-for-story.js node ./scripts/extract-materials.js",


This one is missing a && before node ./scripts/extract-materials.js

dferber90 · 2018-10-23T14:01:45Z

scripts/extract-materials.js

+
+const commentDoNotModify = `/* THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+  This file is created by the 'scripts/extract-materials.js' script. The real
+  values of the variables should be updated in '${relativePath}' */ \n


This resolves to the actual path on the system, so for example /Users/dom/repos/ui-kit/materials/internals. This is bad because it would change every time this file gets generated on a different machine. We need to print the path relative to the root-folder, or just hardcode it.

emmenko

Nice, I like it! Thanks for updating the documentation!

montezume · 2018-10-23T14:39:48Z

Thanks for the help 👍

montezume requested a review from dferber90 October 22, 2018 09:36

montezume self-assigned this Oct 22, 2018

montezume requested review from tdeekens and emmenko October 22, 2018 11:49

montezume added the Semver: FEATURE label Oct 22, 2018

montezume force-pushed the ml-extract-css-vars branch from 19103ee to 014032a Compare October 22, 2018 13:56

emmenko reviewed Oct 22, 2018

View reviewed changes

tdeekens reviewed Oct 23, 2018

View reviewed changes

montezume commented Oct 23, 2018

View reviewed changes

montezume added Semver: BREAKING and removed Semver: FEATURE labels Oct 23, 2018

montezume force-pushed the ml-extract-css-vars branch from 9607d18 to c5a759e Compare October 23, 2018 09:11

montezume commented Oct 23, 2018

View reviewed changes

montezume force-pushed the ml-extract-css-vars branch from cc65ef7 to 32a5957 Compare October 23, 2018 09:53

dferber90 force-pushed the ml-extract-css-vars branch from 32a5957 to 3095e72 Compare October 23, 2018 12:33

dferber90 reviewed Oct 23, 2018

View reviewed changes

dferber90 added 🔮 Type: Chore Semver: FEATURE labels Oct 23, 2018

dferber90 force-pushed the ml-extract-css-vars branch from 99a4fe5 to 726bd43 Compare October 23, 2018 14:25

emmenko approved these changes Oct 23, 2018

View reviewed changes

tdeekens approved these changes Oct 23, 2018

View reviewed changes

montezume and others added 6 commits October 23, 2018 17:30

feat: extract css variables to js and css files

7d81e11

feat: move internal materials internal

9e2aa45

chore: add do not modify block to generated variables file

8129a36

chore: update readme

b2e411d

chore: rename materials exports to imports

2e30f73

chore: reorganize custom-properties

e8778e5

dferber90 force-pushed the ml-extract-css-vars branch from 726bd43 to e8778e5 Compare October 23, 2018 15:31

dferber90 removed the Semver: FEATURE label Oct 23, 2018

dferber90 approved these changes Oct 23, 2018

View reviewed changes

dferber90 merged commit b7c4e85 into master Oct 23, 2018

dferber90 deleted the ml-extract-css-vars branch October 23, 2018 15:36

dferber90 mentioned this pull request Oct 23, 2018

refactor(*-select-input): use JS-based design tokens #182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extract css variables to js and css files #181

feat: extract css variables to js and css files #181

montezume commented Oct 22, 2018 •

edited

Loading

emmenko left a comment

emmenko Oct 22, 2018

montezume commented Oct 23, 2018

tdeekens left a comment

tdeekens Oct 23, 2018

montezume Oct 23, 2018

tdeekens Oct 23, 2018

tdeekens Oct 23, 2018

montezume Oct 23, 2018

tdeekens Oct 23, 2018

montezume Oct 23, 2018

dferber90 Oct 23, 2018

montezume Oct 23, 2018

emmenko Oct 23, 2018

emmenko Oct 23, 2018

dferber90 left a comment •

edited

Loading

dferber90 Oct 23, 2018

dferber90 Oct 23, 2018

emmenko left a comment

montezume commented Oct 23, 2018

feat: extract css variables to js and css files #181

feat: extract css variables to js and css files #181

Conversation

montezume commented Oct 22, 2018 • edited Loading

Summary

Approach.

emmenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume commented Oct 23, 2018

tdeekens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dferber90 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmenko left a comment

Choose a reason for hiding this comment

montezume commented Oct 23, 2018

montezume commented Oct 22, 2018 •

edited

Loading

dferber90 left a comment •

edited

Loading