Add css generating script and package.json hook (2/7) #162

philipfweiss · 2018-06-29T00:37:50Z

Adds a script which will generate a CSS stylesheet from an example component. Based off a similar implementation in https://github.com/airbnb/react-dates.

Sample result of script (inside css/styles.css). Another PR with improvements to/refactoring of the CSS/theming coming soon.

noratarano

Mostly nits! But this is looking great.

noratarano · 2018-06-29T00:44:08Z

scripts/renderAllComponents.jsx

+function App() {
+  return (
+    <LabeledSlider />
+  );


I would just simplify this return <LabeledSlider />;

alternatively, ReactDOM.render(<LabeledSlider /> - no need for App at all.

Yeah, I think simplifying to Jordan's comment seems reasonable.

noratarano · 2018-06-29T00:45:53Z

scripts/buildCSS.js

+
+const path = './scripts/renderAllComponents.jsx';
+
+const CSS = compileCSS(path);


I would avoid this dereference:

const CSS = compileCSS('./scripts/renderAllComponents.jsx');

And also define the format function before this.

const format = new CleanCSS({...}); const CSS = compileCSS('./scripts/renderAllComponents.jsx'); const { styles: formattedCSS } = format.minify(CSS);

Not passionate about this order.

noratarano · 2018-06-29T00:49:52Z

scripts/buildCSS.js

+const args = process.argv.slice(2);
+const optimizeForProduction = args.includes('-o') || args.includes('--optimize');
+
+registerMaxSpecificity(0);


Is it important to run this before registerCSSInterfaceWithDefaultTheme?

I don't think the order matters, as long as it's before the compile script is run. Looks like it just sets a global flag.

https://github.com/airbnb/react-with-styles-interface-css/blob/738405f658e90b254983df3c088c172d2c1786cf/packages/interface/src/utils/registerMaxSpecificity.js

noratarano · 2018-06-29T00:50:01Z

scripts/buildCSS.js

+const compileCSS = require('react-with-styles-interface-css-compiler');
+
+const registerMaxSpecificity = require('react-with-styles-interface-css/dist/utils/registerMaxSpecificity').default;
+const registerCSSInterfaceWithDefaultTheme = require('../src/utils/registerCSSInterfaceWithDefaultTheme').default;


I would bunch up all of these imports without whitespace between them.

hmm - the .default is unfortunate. can this pull from lib instead of src - so it must run after the build process - and then it can require from the babel output, which would have add-module-exports applied?

Is add-module-exports part of the airbnb babel config? We tried to pull from lib and it still needs the .default (also notably, registerMaxSpecificity needs the .default even though it is a built file.

noratarano · 2018-06-29T01:24:03Z

@philipfweiss Can you post a sample result of this script?

ljharb · 2018-06-29T01:28:52Z

package.json

@@ -5,7 +5,10 @@
  "main": "lib/Slider",
  "jsnext:main": "src/Slider.jsx",
  "scripts": {
-    "build": "npm run clean && babel src -d lib",
+    "prebuild": "npm run clean",
+    "build": "npm run clean && babel src -d lib && npm run build:css -- --optimize ",


the clean script can be removed here, since it's in prebuild.

also, let's add a "postbuild" that does npm run build:css, instead of doing it inside the build step?

Isn't the css building part of the build step (just like, semantically).

Maybe let's move the babel src -d lib to a npm run build:js and then change npm run build to be npm run build:js && npm run build:css -- --optimize

oh sure that's fine too

ljharb · 2018-06-29T01:29:42Z

scripts/buildCSS.js

+const compileCSS = require('react-with-styles-interface-css-compiler');
+
+const registerMaxSpecificity = require('react-with-styles-interface-css/dist/utils/registerMaxSpecificity').default;
+const registerCSSInterfaceWithDefaultTheme = require('../src/utils/registerCSSInterfaceWithDefaultTheme').default;


hmm - the .default is unfortunate. can this pull from lib instead of src - so it must run after the build process - and then it can require from the babel output, which would have add-module-exports applied?

ljharb · 2018-06-29T01:30:08Z

scripts/renderAllComponents.jsx

+function App() {
+  return (
+    <LabeledSlider />
+  );


alternatively, ReactDOM.render(<LabeledSlider /> - no need for App at all.

ljharb · 2018-06-29T01:30:39Z

src/utils/.eslintrc

+{
+  "rules": {
+    "import/no-extraneous-dependencies": [2, {
+      "devDependencies": true


should these utils maybe live inside scripts/, since they're not for public consumption?

well actually they might be? as part of the eventual initialize script.

if that's the case then they definitely shouldn't be allowed to have dev deps :-)

majapw · 2018-06-29T17:40:50Z

package.json

@@ -5,7 +5,10 @@
  "main": "lib/Slider",
  "jsnext:main": "src/Slider.jsx",
  "scripts": {
-    "build": "npm run clean && babel src -d lib",
+    "prebuild": "npm run clean",
+    "build": "npm run clean && babel src -d lib && npm run build:css -- --optimize ",


Isn't the css building part of the build step (just like, semantically).

Maybe let's move the babel src -d lib to a npm run build:js and then change npm run build to be npm run build:js && npm run build:css -- --optimize

majapw · 2018-06-29T17:41:25Z

package.json

@@ -54,6 +58,8 @@
    "react-addons-pure-render-mixin": "^15.6.2",
    "react-dom": "^15.6.2",
    "react-with-styles-interface-aphrodite": "^5.0.0",
+    "react-with-styles-interface-css": "^4.0.2",


Since this is going to be used by the consumer as part of the initialize script, it will need to be part of the regular dependencies

majapw · 2018-06-29T17:42:15Z

scripts/buildCSS.js

+const compileCSS = require('react-with-styles-interface-css-compiler');
+
+const registerMaxSpecificity = require('react-with-styles-interface-css/dist/utils/registerMaxSpecificity').default;
+const registerCSSInterfaceWithDefaultTheme = require('../src/utils/registerCSSInterfaceWithDefaultTheme').default;


Is add-module-exports part of the airbnb babel config? We tried to pull from lib and it still needs the .default (also notably, registerMaxSpecificity needs the .default even though it is a built file.

majapw · 2018-06-29T17:43:19Z

scripts/renderAllComponents.jsx

+function App() {
+  return (
+    <LabeledSlider />
+  );


Yeah, I think simplifying to Jordan's comment seems reasonable.

majapw · 2018-06-29T17:43:58Z

src/utils/.eslintrc

+{
+  "rules": {
+    "import/no-extraneous-dependencies": [2, {
+      "devDependencies": true


well actually they might be? as part of the eventual initialize script.

majapw · 2018-06-29T17:44:21Z

src/utils/registerInterfaceWithDefaultTheme.js

+import ThemedStyleSheet from 'react-with-styles/lib/ThemedStyleSheet';
+import DefaultTheme from '../themes/DefaultTheme';
+
+export default function registerInterfaceWithDefaultTheme(reactWithStylesInterface) {


it'd be good to change the test setup and storybook config to use this instead maybe

majapw

This looks great to me! I have a few small comments and then I think it's ready to merge.

majapw · 2018-07-02T17:21:41Z

package.json

    "build": "npm run clean && babel src -d lib",
+    "postbuild": "npm run build:css",


I still prefer to have:

"build:js": "babel src -d lib",

as a separate script and then have "build" be "npm run build:js && npm run build:css". The CSS step is not a postbuild, it's part of the build itself.

majapw · 2018-07-02T17:21:53Z

package.json

@@ -5,7 +5,11 @@
  "main": "lib/Slider",
  "jsnext:main": "src/Slider.jsx",
  "scripts": {
+    "prebuild": "npm run clean",
    "build": "npm run clean && babel src -d lib",


We can remove this npm run clean since it's in the prebuild now.

majapw · 2018-07-02T17:22:46Z

package.json

@@ -63,9 +68,11 @@
  },
  "dependencies": {
    "airbnb-prop-types": "^2.10.0",
+    "clean-css": "^4.1.11",


clean-css should be a dev dep

majapw · 2018-07-02T17:23:46Z

scripts/buildCSS.js

+const registerCSSInterfaceWithDefaultTheme = require('./utils/registerCSSInterfaceWithDefaultTheme').default;
+
+const args = process.argv.slice(2);
+const optimizeForProduction = args.includes('-o') || args.includes('--optimize');


not critical, but it might be cleaner to update this to use yargs or something.

If we’re going to do that, maybe this should be a command line util in the css interface repo?

majapw · 2018-07-02T17:26:10Z

scripts/buildCSS.js

+  fs.mkdirSync(dir);
+}
+
+const outputFilePath = optimizeForProduction ? `${dir}/_styles.css` : `${dir}/styles.css`;


the underscore is probably not necessary (it was there for some backwards compatibility w/ react-dates). It might be nice to name this file rheostat.css so that it is easy to understand in the context of a bigger app.

majapw · 2018-07-02T17:30:08Z

.storybook/config.js

-import '../css/slider-horizontal.css';
-import '../css/slider-vertical.css';
+/* Register react with styles interface */
+ThemedStyleSheet.registerTheme(DefaultTheme);


Let's use utils/registerInterfaceWithDefaultTheme.js here with the aphroditeInterface. :)

Addressed!

majapw

LGTM except for the one comment

majapw · 2018-07-02T22:33:27Z

package.json

+    "build": "npm run build:js && npm run build:css",
+    "build:js": "babel src -d lib",
+    "prebuild:css": "rimraf lib/css && mkdir -p lib/css",
+    "build:css": "node scripts/buildCSS.js",
    "clean": "rimraf lib",
    "prepublish": "in-publish && safe-publish-latest && npm run build || not-in-publish",
    "lint": "eslint --ext .js,.jsx src test stories",


Do we want to add a script for running the css storybook? e.g.

"storybook:css": "npm run build:css && start-storybook -p 9001 -c .storybook-css",

Have this in a later commit! #166

philipfweiss requested review from majapw, ljharb and noratarano June 29, 2018 00:37

noratarano reviewed Jun 29, 2018

View reviewed changes

ljharb previously requested changes Jun 29, 2018

View reviewed changes

majapw reviewed Jun 29, 2018

View reviewed changes

philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch 2 times, most recently from e10ddba to 3fd8ae1 Compare June 29, 2018 17:55

philipfweiss mentioned this pull request Jun 29, 2018

Clean up rheostat css theming (3/7) #163

Merged

philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch from 3fd8ae1 to 5ecf6e1 Compare June 29, 2018 18:30

philipfweiss mentioned this pull request Jun 29, 2018

[test] only run build script on latest node version (1/7) #165

Merged

philipfweiss changed the title ~~Add css generating script and package.json hook~~ Add css generating script and package.json hook (2/7) Jun 29, 2018

philipfweiss changed the base branch from master to pw--move-build-job June 29, 2018 23:30

philipfweiss changed the base branch from pw--move-build-job to master June 29, 2018 23:31

philipfweiss mentioned this pull request Jun 30, 2018

Add storybook css interface (4/7) #164

Merged

philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch 5 times, most recently from ca50ace to 7f4a8eb Compare July 1, 2018 21:56

majapw reviewed Jul 2, 2018

View reviewed changes

add css generating script and package.json hook

e5cb7f8

philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch from 7f4a8eb to e5cb7f8 Compare July 2, 2018 18:49

majapw approved these changes Jul 2, 2018

View reviewed changes

philipfweiss merged commit b42a856 into master Jul 2, 2018

ljharb deleted the pw--rheostat-add-css-building-scripts branch July 2, 2018 22:48


		const path = './scripts/renderAllComponents.jsx';

		const CSS = compileCSS(path);

		"build": "npm run clean && babel src -d lib",
		"postbuild": "npm run build:css",

Add css generating script and package.json hook (2/7) #162

Add css generating script and package.json hook (2/7) #162

Conversation

philipfweiss commented Jun 29, 2018 • edited Loading

noratarano 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

noratarano commented Jun 29, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw 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

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipfweiss commented Jun 29, 2018 •

edited

Loading