Update deps & modernize module #178
Conversation
Zertz
commented
Jun 28, 2018
Zertz
commented
- Convert CoffeeScript to plain old JavaScript
- Update Autoprefixer
- Update PostCSS
- Install Prettier
- Update tests
|
@jescalan Anything we can do to move this project forward? |
.npmignore
Outdated
| @@ -1,3 +1,2 @@ | |||
| test | |||
| .editorconfig | |||
There was a problem hiding this comment.
Why would you delete the editorconfig file?
There was a problem hiding this comment.
I do - editorconfigs are a good way to stay in sync with editor-level settings like indentation etc no matter which text editor you are using. There is an argument to be made for prettier superseding it in this case though, as long as prettier running per-commit is made mandatory.
.vscode/settings.json
Outdated
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "editor.formatOnSave": true | |||
| } | |||
There was a problem hiding this comment.
I'd prefer if personal config files were kept out of the primary source if possible
LICENSE.md
Outdated
| @@ -1,5 +1,4 @@ | |||
| License (MIT) | |||
| ------------- | |||
| ## License (MIT) | |||
There was a problem hiding this comment.
Why change this to an h2? It is the primary heading...
| "coveralls": "2.13.1", | ||
| "css-parse": "2.0.0", | ||
| "istanbul": "0.4.5", | ||
| "mocha": "3.4.1", | ||
| "mocha-lcov-reporter": "1.3.0", | ||
| "prettier": "^1.13.7", |
There was a problem hiding this comment.
Prettier is a dep but is not actually used anywhere (other than perhaps your personal vscode settings). If you'd like for the project to use prettier for formatting, it needs to be implemented somehow. Typically I use a "format" npm script set up with a pre-commit hook.
| @@ -1,2 +1,2 @@ | |||
| p | |||
| display: flex | |||
| display flex | |||
There was a problem hiding this comment.
Why remove the colon here? I prefer using colons even within stylus to make the key/value split clear and the code more understandable.
There was a problem hiding this comment.
I agree on readability, the change was mostly because stylus allows it and to make sure it's correctly transpiled.
There was a problem hiding this comment.
This library doesn't need to test stylus' features - the fact that there are no brackets is already enough to separate it from normal css and ensure it is being transpiled 😀
| done(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Would prefer to commit a package-lock.json rather than a yarn lock, as npm is the more frequently used option and the default for node.
There was a problem hiding this comment.
Good catch, it shouldn't actually even be here! Apps should manage their lock file and not libraries, I'll remove it entirely.
|
Made most of these changes directly - thank you for the contribution! |
