-
Notifications
You must be signed in to change notification settings - Fork 139
feat(support-various-styling): add support for 3 styling types (closes #4) #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ const os = require('os'); | |
| const prettier = require('prettier'); | ||
| const chalk = require('chalk'); | ||
|
|
||
| const { requireOptional } = require('./utils'); | ||
| const { requireOptional } = require('../utils'); | ||
|
|
||
|
|
||
| // Get the configuration for this component. | ||
|
|
@@ -30,6 +30,7 @@ module.exports.getConfig = () => { | |
|
|
||
| const defaults = { | ||
| type: 'class', | ||
| styling: 'none', | ||
| dir: 'src/components', | ||
| extension: 'js' | ||
| }; | ||
|
|
@@ -46,6 +47,24 @@ module.exports.buildPrettifier = prettierConfig => text => ( | |
| prettier.format(text, prettierConfig) | ||
| ); | ||
|
|
||
| const componentTypes = { | ||
| FUNCTIONAL: 'functional', | ||
| CLASS: 'class', | ||
| PURE_CLASS: 'pure-class', | ||
| }; | ||
| module.exports.componentTypes = componentTypes; | ||
|
|
||
| const stylingTypes = { | ||
| STYLED: 'styled-components', | ||
| CSS_MODULES: 'css-modules', | ||
| APHRODITE: 'aphrodite', | ||
| }; | ||
| module.exports.stylingTypes = stylingTypes; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think we can just do My reasoning is that I'm so used to ES6 exports... the fact that it's
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
They were like what you said, but later I discovered we'll need these variables here for example. Can we reference these by
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| module.exports.hasCssFile = styling => { | ||
| return styling === module.exports.stylingTypes.CSS_MODULES; | ||
| } | ||
|
|
||
| // Emit a message confirming the creation of the component | ||
| const colors = { | ||
| red: [216, 16, 16], | ||
|
|
@@ -57,7 +76,17 @@ const colors = { | |
| }; | ||
|
|
||
| const logComponentType = (selected) => ( | ||
| ['class', 'pure-class', 'functional'] | ||
| Object.values(componentTypes) | ||
| .sort((a, b) => a === selected ? -1 : 1) | ||
| .map(option => ( | ||
| option === selected | ||
| ? `${chalk.bold.rgb(...colors.blue)(option)}` | ||
| : `${chalk.rgb(...colors.darkGray)(option)}` | ||
| )).join(' ') | ||
| ); | ||
|
|
||
| const logStylingType = (selected) => ( | ||
| Object.values(stylingTypes) | ||
| .sort((a, b) => a === selected ? -1 : 1) | ||
| .map(option => ( | ||
| option === selected | ||
|
|
@@ -66,17 +95,21 @@ const logComponentType = (selected) => ( | |
| )).join(' ') | ||
| ); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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);
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good refactoring tip. I'll do better with refactoring later on. |
||
|
|
||
| module.exports.logIntro = ({ name, dir, type }) => { | ||
| module.exports.logIntro = ({ name, dir, type, styling }) => { | ||
| console.info('\n'); | ||
| console.info(`✨ Creating the ${chalk.bold.rgb(...colors.gold)(name)} component ✨`); | ||
| console.info('\n'); | ||
|
|
||
|
|
||
| const pathString = chalk.bold.rgb(...colors.blue)(dir); | ||
| const typeString = logComponentType(type); | ||
| const componentTypeString = logComponentType(type); | ||
| const stylingTypeString = logStylingType(styling); | ||
|
|
||
| console.info(`Directory: ${pathString}`); | ||
| console.info(`Type: ${typeString}`); | ||
| console.info(`Type: ${componentTypeString}`); | ||
| if (styling && styling !== 'none') { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I talked more about this above, but yeah I think we should log the styling even when "none" is selected :) |
||
| console.info(`Styling: ${stylingTypeString}`); | ||
| } | ||
| console.info(chalk.rgb(...colors.darkGray)('=========================================')); | ||
|
|
||
| console.info('\n'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| const reactImportTemplates = require('../templates/react-imports'); | ||
| const styleImportTemplates = require('../templates/style-imports'); | ||
| const renderContentTemplates = require('../templates/render-contents'); | ||
| const componentDefinitionTemplates = require('../templates/component-definition'); | ||
| const belowComponentTemplates = require('../templates/below-component'); | ||
| const exportsTemplates = require('../templates/exports'); | ||
|
|
||
| module.exports = (componentName, componentType, style) => { | ||
| const renderContents = renderContentTemplates(style); | ||
| const template = ` | ||
| ${reactImportTemplates(componentType)} | ||
| ${styleImportTemplates(style, componentName)} | ||
|
|
||
| ${componentDefinitionTemplates(componentType, componentName, renderContents)} | ||
|
|
||
| ${belowComponentTemplates(style)} | ||
|
|
||
| ${exportsTemplates(style, componentName)} | ||
| `; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| return template; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,14 +11,16 @@ const { | |
| logItemCompletion, | ||
| logConclusion, | ||
| logError, | ||
| hasCssFile, | ||
| } = require('./helpers'); | ||
| const { | ||
| requireOptional, | ||
| mkDirPromise, | ||
| readFilePromiseRelative, | ||
| writeFilePromise, | ||
| } = require('./utils'); | ||
|
|
||
| const generateTemplate = require('./helpers/templateConstructor'); | ||
| const cssTemplate = require('./templates/css'); | ||
|
|
||
| // Load our package.json, so that we can pass the version onto `commander`. | ||
| const { version } = require('../package.json'); | ||
|
|
@@ -30,6 +32,9 @@ const config = getConfig(); | |
| // Convenience wrapper around Prettier, so that config doesn't have to be | ||
| // passed every time. | ||
| const prettify = buildPrettifier(config.prettierConfig); | ||
| const prettifyCss = buildPrettifier(Object.assign({}, config.prettierConfig, { | ||
| parser: 'postcss' | ||
| })); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome! Thanks so much for prettifying the CSS, had no idea Prettier could do that 🔥 🔥 🔥 |
||
|
|
||
|
|
||
| program | ||
|
|
@@ -40,6 +45,11 @@ program | |
| 'Type of React component to generate (default: "class")', | ||
| /^(class|pure-class|functional)$/i, | ||
| config.type | ||
| ).option( | ||
| '-s, --styling <stylingSolution>', | ||
| 'Which styling solution to generate for (default: "none")', | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copy nit: Can we make this
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! You know English better than me for SURE :) |
||
| /^(none|styled-components|css-modules|aphrodite)$/i, | ||
| config.styling | ||
| ).option( | ||
| '-d, --dir <pathToDirectory>', | ||
| 'Path to the "components" directory (default: "src/components")', | ||
|
|
@@ -52,20 +62,23 @@ program | |
|
|
||
| const [componentName] = program.args; | ||
|
|
||
| // Find the path to the selected template file. | ||
| const templatePath = `./templates/${program.type}.js`; | ||
|
|
||
| // Get all of our file paths worked out, for the user's project. | ||
| const componentDir = `${program.dir}/${componentName}`; | ||
| const filePath = `${componentDir}/${componentName}.${program.extension}`; | ||
| const componentFilePath = `${componentDir}/${componentName}.${program.extension}`; | ||
| const cssFilePath = `${componentDir}/${componentName}.${'css'}`; | ||
| const indexPath = `${componentDir}/index.js`; | ||
|
|
||
| // Our index template is super straightforward, so we'll just inline it for now. | ||
| const indexTemplate = prettify(`\ | ||
| const indexTemplate = `\ | ||
| export { default } from './${componentName}'; | ||
| `); | ||
| `; | ||
|
|
||
| logIntro({ name: componentName, dir: componentDir, type: program.type }); | ||
| logIntro({ | ||
| name: componentName, | ||
| dir: componentDir, | ||
| type: program.type, | ||
| styling: program.styling, | ||
| }); | ||
|
|
||
|
|
||
| // Check if componentName is provided | ||
|
|
@@ -91,33 +104,37 @@ if (fs.existsSync(fullPathToComponentDir)) { | |
| // Start by creating the directory that our component lives in. | ||
| mkDirPromise(componentDir) | ||
| .then(() => ( | ||
| readFilePromiseRelative(templatePath) | ||
| generateTemplate(componentName, program.type, program.styling) | ||
| )) | ||
| .then(template => { | ||
| logItemCompletion('Directory created.'); | ||
| return template; | ||
| }) | ||
| .then(template => ( | ||
| // Replace our placeholders with real data (so far, just the component name) | ||
| template.replace(/COMPONENT_NAME/g, componentName) | ||
| )) | ||
| .then(template => ( | ||
| // Format it using prettier, to ensure style consistency, and write to file. | ||
| writeFilePromise(filePath, prettify(template)) | ||
| writeFilePromise(componentFilePath, prettify(template)) | ||
| )) | ||
| .then(template => { | ||
| .then(() => { | ||
| // Check whether we should make a CSS file too | ||
| if (hasCssFile(program.styling)) { | ||
| // Format it using prettier, to ensure style consistency, and write to file. | ||
| return writeFilePromise(cssFilePath, prettifyCss(cssTemplate)) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think it'd be better to move the line that defines Just because it isn't used most of the time, so it feels a little cleaner to not have this unused constant defined for every execution.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be like what you said! But I thought maybe you'll like to put it beside other paths variables :) Sorry! |
||
| .then(() => { | ||
| logItemCompletion('CSS file built and saved to disk.'); | ||
| }); | ||
| } | ||
| }) | ||
| .then(() => { | ||
| logItemCompletion('Component built and saved to disk.'); | ||
| return template; | ||
| }) | ||
| .then(template => ( | ||
| .then(() => ( | ||
| // We also need the `index.js` file, which allows easy importing. | ||
| writeFilePromise(indexPath, prettify(indexTemplate)) | ||
| )) | ||
| .then(template => { | ||
| .then(() => { | ||
| logItemCompletion('Index file built and saved to disk.'); | ||
| return template; | ||
| }) | ||
| .then(template => { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Didn't realize I was pointlessly passing the template down :)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was confused for a few moments why we're doing this :)) |
||
| .then(() => { | ||
| logConclusion(); | ||
| }) | ||
| .catch(err => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| const { stylingTypes } = require('../helpers'); | ||
|
|
||
| const styled = `\ | ||
| const Wrapper = styled.div\` | ||
| \`; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, Prettier will make it like so:
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok! No problem then :) |
||
| `; | ||
|
|
||
| const aphrodite = `\ | ||
| const styles = StyleSheet.create({ | ||
| wrapper: { | ||
| }, | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Same note as above; if possible with Prettier, can we add a blank line between open and close curlies?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: {
height: 'auto',
width: 'auto',
}
}What do you think? Happy to discuss!
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| }); | ||
| `; | ||
|
|
||
| module.exports = (style, componentType) => { | ||
| switch (style) { | ||
| case stylingTypes.STYLED: | ||
| return styled; | ||
|
|
||
| case stylingTypes.APHRODITE: | ||
| return aphrodite; | ||
|
|
||
| default: | ||
| return ''; | ||
| } | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| const { componentTypes } = require('../helpers'); | ||
|
|
||
| const functional = (ComponentName, renderContents) => (` | ||
| const ${ComponentName} = () => { | ||
| return ${renderContents}; | ||
| }; | ||
| `) | ||
|
|
||
| const classComp = (ComponentName, renderContents) => (` | ||
| class ${ComponentName} extends Component { | ||
| render() { | ||
| return ${renderContents}; | ||
| } | ||
| } | ||
| `) | ||
|
|
||
| const pureClass = (ComponentName, renderContents) => (` | ||
| class ${ComponentName} extends PureComponent { | ||
| render() { | ||
| return ${renderContents}; | ||
| } | ||
| } | ||
| `) | ||
|
|
||
| module.exports = (componentType, ...args) => { | ||
| switch (componentType) { | ||
| case componentTypes.FUNCTIONAL: | ||
| return functional(...args); | ||
|
|
||
| case componentTypes.CLASS: | ||
| return classComp(...args); | ||
|
|
||
| case componentTypes.PURE_CLASS: | ||
| return pureClass(...args); | ||
| } | ||
| } | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great method! Good use of rest/spread :) |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| module.exports = ` | ||
| .wrapper { | ||
| } | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: same note as above, add a blank line if possible |
||
| `; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| const { stylingTypes } = require('../helpers'); | ||
|
|
||
| module.exports = (style, componentName) => { | ||
| switch (style) { | ||
| default: | ||
| return `export default ${componentName};`; | ||
| } | ||
| } |
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| const { componentTypes } = require('../helpers'); | ||
|
|
||
| module.exports = (componentType) => { | ||
| switch (componentType) { | ||
| case componentTypes.FUNCTIONAL: | ||
| return "import React from 'react';"; | ||
|
|
||
| case componentTypes.CLASS: | ||
| return "import React, { Component } from 'react';"; | ||
|
|
||
| case componentTypes.PURE_CLASS: | ||
| return "import React, { PureComponent } from 'react';"; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| const { stylingTypes } = require('../helpers'); | ||
|
|
||
| module.exports = (style) => { | ||
| switch (style) { | ||
| case stylingTypes.STYLED: | ||
| return '<Wrapper />'; | ||
|
|
||
| case stylingTypes.CSS_MODULES: | ||
| return '<div className={styles.wrapper} />'; | ||
|
|
||
| case stylingTypes.APHRODITE: | ||
| return '<div className={css(styles.wrapper)} />'; | ||
|
|
||
| default: | ||
| return '<div />'; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| const { stylingTypes } = require('../helpers'); | ||
|
|
||
| module.exports = (style, componentName) => { | ||
| switch (style) { | ||
| case stylingTypes.STYLED: | ||
| return "import styled from 'styled-components';"; | ||
|
|
||
| case stylingTypes.CSS_MODULES: | ||
| return `import styles from './${componentName}.css';`; | ||
|
|
||
| case stylingTypes.APHRODITE: | ||
| return "import { StyleSheet, css } from 'aphrodite';"; | ||
|
|
||
| default: | ||
| return ''; | ||
| } | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we add case stylingTypes.NONE:
default:
return '';By using the fallthrough we can make
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. This (the fallthrough) was the reason I suggested using |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider
nonea 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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! When you said you want to keep the project as simple as possible, I thought showing the
styling: none styled css-modules aphroditein console every time would confuse beginner users.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)