zombieFox / nightTab

A neutral new tab page accented with a chosen colour. Customise the layout, style, background and bookmarks with nightTab.
https://zombiefox.github.io/nightTab/
GNU General Public License v3.0
1.78k stars 254 forks source link

Add eslint and huskyjs to catch bugs and maintain consistent code style #294

Closed metruzanca closed 3 years ago

metruzanca commented 3 years ago

As the title states, this PR implements eslint and huskyjs to catch bugs early and maintain consistent code style.

To break this down futher

Eslint

Eslint is a tool many javascript projects use to catch bugs early, eslint says this on their home page:

ESLint statically analyzes your code to quickly find problems. ESLint is built into most text editors and you can run ESLint as part of your continuous integration pipeline. Many problems ESLint finds can be automatically fixed. ESLint fixes are syntax-aware so you won't experience errors introduced by traditional find-and-replace algorithms. [...] -eslint.org (homepage descrription)

Many problems in javascript are caused by the fact that javascript lets you do anything, most of the time to a fault and to many footguns. Eslint is a sort of guardrail to stop you from hurting yourself when you inevitably bang your head against the desk over a silly bug.

As for "maintain consistent code style" this is something thats paramount if this is to be an open source project that allows contributions. This way anyone forking this project is forced to use the shared style.

Huskyjs

Huskyjs is a git hook manager. This PR implements a pre-commit hook that runs npm run lint before every commit. If the linting fails, your commit is aborted. (eslint will try to fix easy problems on the fly, but anything requiring manual intervention will fail)

To avoid having to run eslint along side the dev server, I recommend installing the eslint vscode extension (or the equivalent for your editor/IDE) with this configuration:

{
    "editor.codeActionsOnSave": {
        "source.fixAll.eslint": true
    },
    "eslint.validate": [
        "javascript",
        "typescript"
    ]
}

This will trigger eslint --fix for the file you're editing on save which uses the .eslintrc file in the repo. (Anything you may have set in vscode extention settings will override IIRC, so becareful. I recommend not touching any other extension settings in eslint for ease of use)


EDIT:

metruzanca commented 3 years ago

Re-wrote history to re-include the semi-colons, thats why you see force-push with a bunch of extra commits.

zombieFox commented 3 years ago

Still working through the changes and testing CSS -- but I found a bug. The header items can no longer be sorted with drag and drop:

https://user-images.githubusercontent.com/7115017/135807225-bc1bebd6-cfed-4da2-b3da-8f984c78d4db.mp4

metruzanca commented 3 years ago

Still working through the changes and testing CSS -- but I found a bug. The header items can no longer be sorted with drag and drop: Kapture.2021-10-04.at.07.58.15.mp4

Going to try to figure out the cause today, I'll get back to you on this.

metruzanca commented 3 years ago

This (0592eb4) is the commit that contains the breaking change.

metruzanca commented 3 years ago

Fixed, pushing in a sec.

src/component/header/index.js - L407

This was my bad. I stripped out Sortable.create because the result was stored in a variable which was unused. I should have stripped the variable definition but kept the sortablejs call.

zombieFox commented 3 years ago

I feel the reset CSS file should not live in src/component/index.css. It is a base file and it helps set the context if it lives in src/component/base/reset/index.css. I ran tests with npm run build to make sure the CSS is loaded in the correct order and it does. Can we move the rest.css file to this new location?

zombieFox commented 3 years ago

Small thing, but can we change src/constants to src/constant. This will keep the directories consistent, (in singular format).

metruzanca commented 3 years ago

I feel the reset CSS file should not live in src/component/index.css. It is a base file and it helps set the context if it lives in src/component/base/reset/index.css. I ran tests with npm run build to make sure the CSS is loaded in the correct order and it does. Can we move the rest.css file to this new location?

Well in that case I'd propose we move it outside of components, possibly to a src/styles/ directory. For 2 mains reasons:

Heres an example of a reactapp's folder structure image And heres an example of a vueapp image

metruzanca commented 3 years ago

Small thing, but can we change src/constants to src/constant. This will keep the directories consistent, (in singular format).

Sure.

metruzanca commented 3 years ago

Ahh oops. I thought that would resolve the comment

zombieFox commented 3 years ago

Well in that case I'd propose we move it outside of components, possibly to a src/styles/ directory.

I like this. Happy to move src/component/base/... to something like src/style/... so long as the styles inside are loaded first to maintain the correct style inheritance.

I can handle that in outside this PR after this is merged if you would like.

metruzanca commented 3 years ago

so long as the styles inside are loaded first to maintain the correct style inheritance.

Well of course, we want the app to work after this PR, don't we? Merely a code organization change.

I can handle that in outside this PR after this is merged if you would like.

Nah, I'll include it now. I'm fixing all the things we're commenting.