feat(support-various-styling): add support for 3 styling types (closes #4) by morajabi · Pull Request #9 · joshwcomeau/new-component

morajabi · 2017-09-20T13:28:13Z

Hey, another one today!

This one was a bit more complicated, it needed some serious (🤔) structural changes to the codebase. I'm in the middle of a project but I really like this tool and started to do it! so I hope you like it. Most of it is based on your awesome idea (Thanks, @joshwcomeau ❤️) + some tweaks to optimize and future proof it.

I added 3 styling solutions for now (we can add more easily later!):

Styled-Components

Sample code:

// Hello.js
import React, { Component } from "react";
import styled from "styled-components";

class Hello extends Component {
  render() {
    return <Wrapper />;
  }
}

const Wrapper = styled.div``;

export default Hello;

Aphrodite

Sample code:

// HelloAph.js
import React, { Component } from "react";
import { StyleSheet, css } from "aphrodite";

class HelloAph extends Component {
  render() {
    return <div className={css(styles.wrapper)} />;
  }
}

const styles = StyleSheet.create({
  wrapper: {}
});

export default HelloAph;

CSS Modules

Sample code:

// HelloCSS.js
import React, { Component } from "react";
import styles from "./HelloCSS.css";

class HelloCSS extends Component {
  render() {
    return <div className={styles.wrapper} />;
  }
}

export default HelloCSS;

And

/* HelloCSS.css */
.wrapper {
}

.
.
Now I'm gonna use it everyday ✌️😋

morajabi · 2017-09-20T13:28:58Z

BTW, Sorry I was deep inside the code and forgot to commit features one at a time

joshwcomeau

Omg this is awesome!!! Thanks so so much for your work on it, quality is fantastic. Both your PRs went above and beyond the requirements, so I really appreciate it :)

Don't worry about it all being 1 commit; I like to squash PRs before merging anyway, so it makes no difference to me.

I'm requesting changes for 1 main reason, and that's to add none as a valid styling type (where the templates would just do nothing, but support it as an option), where we still log it out.

I have a bunch of tiny nitpicks as well, but they should be really quick to implement.

I know you're busy so feel free to hand off this work to me, I can do the changes I requested. I'm in no rush, though, so if you want to wait and tackle it later, that's 100% cool with me as well.

Thanks Mohammad!

joshwcomeau · 2017-09-23T10:55:48Z

src/helpers/index.js

+  CSS_MODULES: 'css-modules',
+  APHRODITE: 'aphrodite',
+};
+module.exports.stylingTypes = stylingTypes;


Nit: I think we can just do module.exports.componentTypes = { ... }. Same for stylingTypes.

My reasoning is that I'm so used to ES6 exports... the fact that it's const stylingTypes and not export const stylingTypes made me think it wasn't exported... so having it connected directly to module.exports = would make it easier to tell at a glance.

My reasoning is that I'm so used to ES6 exports
Me too.

They were like what you said, but later I discovered we'll need these variables here for example. Can we reference these by modules.exports.componentTypes?

Ohh, I see. Didn't realize that. Yeah, I'd leave this as-is then :)

EDIT: For what it's worth, you can reference them that way. but it feels a little funny to me.

joshwcomeau · 2017-09-23T10:58:57Z

src/helpers/index.js

@@ -66,17 +95,21 @@ const logComponentType = (selected) => (
    )).join('  ')
 );


I think there's enough duplication that an abstraction could come in handy here :)

const logSelected = (values, selected) => ( Object.values(values) .sort((a, b) => a === selected ? -1 : 1) .map(option => ( option === selected ? `${chalk.bold.rgb(...colors.blue)(option)}` : `${chalk.rgb(...colors.darkGray)(option)}` )).join(' ') );

Then you could just use it:

const componentTypeString = logSelected(componentTypes, type); const stylingTypeString = logSelected(stylingTypes, styling);

Good refactoring tip. I'll do better with refactoring later on.

joshwcomeau · 2017-09-23T11:02:50Z

src/helpers/templateConstructor.js

+  ${belowComponentTemplates(style)}
+
+  ${exportsTemplates(style, componentName)}
+  `;


Nit: So with the templates (even the ones I wrote), we don't indent the template string contents:

const template = ` stuff `;

Initially this was because I didn't want the exported file having a bunch of leading whitespace... but now that I think about it, I think prettier would trim all that, so it wouldn't be a problem.

In this case though, because it's already indented by the module.exports function, I think it's fine to indent:

module.exports = (componentName, componentType, style) => { const template = ` stuff `; }

No change necessary for the other template strings though (unless you think it'd be better; I'm happy either way with the stuff in templates/component-definition.js)

joshwcomeau · 2017-09-23T11:05:40Z

src/helpers/index.js

+  STYLED: 'styled-components',
+  CSS_MODULES: 'css-modules',
+  APHRODITE: 'aphrodite',
+};


I think we should consider none a valid styling type.

My reasoning is that the default styling is no styling. Someone may install this project, skim the README too quickly, and not even realize that it's capable of doing styling!

By showing that "None" is the styling option they've selected, we also show the options they haven't selected. They might say "oh cool this does styled-components! I need that".

OK! When you said you want to keep the project as simple as possible, I thought showing the styling: none styled css-modules aphrodite in console every time would confuse beginner users.

Yeah, my fault for not specifying! I can see the argument of wanting to reduce the noise as much as possible, but I think that ultimately it's worth the added line of info :)

joshwcomeau · 2017-09-23T11:06:05Z

src/helpers/index.js

  console.info(`Directory:  ${pathString}`);
-  console.info(`Type:       ${typeString}`);
+  console.info(`Type:       ${componentTypeString}`);
+  if (styling && styling !== 'none') {


I talked more about this above, but yeah I think we should log the styling even when "none" is selected :)

joshwcomeau · 2017-09-23T11:20:05Z

src/index.js

    logItemCompletion('Index file built and saved to disk.');
-    return template;
  })
-  .then(template => {


Good catch! Didn't realize I was pointlessly passing the template down :)

I was confused for a few moments why we're doing this :))

joshwcomeau · 2017-09-23T11:20:41Z

src/templates/below-component.js

+const aphrodite = `\
+const styles = StyleSheet.create({
+  wrapper: {
+  },


Nit: Same note as above; if possible with Prettier, can we add a blank line between open and close curlies?

I think as Prettier introduces itself as an opinionated code formatter, it shouldn't be possible :) (Also I couldn't find an option to avoid it here)

But maybe we can write a placeholder style for wrapper as create-react-app does that when you first init a project. I mean something like this:

{ wrapper: { height: 'auto', width: 'auto', } }

What do you think? Happy to discuss!

Its ok :) no change necessary, I'd rather not apply any styles, and it's super easy for a developer to just add that new line themselves when they want to add styles.

joshwcomeau · 2017-09-23T11:21:40Z

src/templates/below-component.js

+
+const styled = `\
+const Wrapper = styled.div\`
+\`;


Nit: can you add a blank line inside the template string?

Will Prettier strip it out, if you do? I like the idea of having a space for someone to add their styles, but it's not a big deal :)

Will Prettier strip it out, if you do?

Yeah, Prettier will make it like so:

const Wrapper = styled.div``

ok! No problem then :)

joshwcomeau · 2017-09-23T11:22:44Z

src/templates/component-definition.js

+    case componentTypes.PURE_CLASS:
+      return pureClass(...args);
+  }
+}


Great method! Good use of rest/spread :)

joshwcomeau · 2017-09-23T11:23:00Z

src/templates/css.js

@@ -0,0 +1,4 @@
+module.exports = `
+.wrapper {
+}


Nit: same note as above, add a blank line if possible

feat(support-various-styling): add support for 3 styling types

8888c82

morajabi changed the title ~~feat(support-various-styling): add support for 3 styling types~~ feat(support-various-styling): add support for 3 styling types (closes #4) Sep 20, 2017

morajabi requested a review from joshwcomeau September 20, 2017 14:40

joshwcomeau requested changes Sep 23, 2017

View reviewed changes

		@@ -66,17 +95,21 @@ const logComponentType = (selected) => (
		)).join(' ')
		);

Conversation

morajabi commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Styled-Components

Aphrodite

CSS Modules

Uh oh!

morajabi commented Sep 20, 2017

Uh oh!

joshwcomeau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshwcomeau Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morajabi Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morajabi Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

morajabi commented Sep 20, 2017 •

edited

Loading

joshwcomeau Oct 11, 2017 •

edited

Loading

morajabi Sep 23, 2017 •

edited

Loading

morajabi Sep 23, 2017 •

edited

Loading