Skip to content
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

Adding to precommit hooks #649

Open
garyb1 opened this issue Nov 24, 2023 · 6 comments
Open

Adding to precommit hooks #649

garyb1 opened this issue Nov 24, 2023 · 6 comments

Comments

@garyb1
Copy link
Contributor

garyb1 commented Nov 24, 2023

Open question about precommit hooks. We can create individual PR's if we agree.

  • Unit Tests? Obviously need to improve testing as a whole but it can be good to ensure tests are ran.
  • Linting with Eslint - We have a lint script but it does not get activated yet. Need another ticket to address linting issues. Mainly a11y and TS errors/warnings.
  • Markdown spellcheck
  • JSHint
  • CSS Linting: A CSS linter that helps you avoid errors and enforce conventions.
  • Commitlint helps your team adhere to a commit convention.

Feel free to comment below.

@NiallJoeMaher
Copy link
Contributor

Some amazing ideas here!

This is a preference because I like people to commit early and often (even if it's not 100%). However, pull requests should not be merged without these checks passing.

My preference for these things would be individual GitHub actions that confirm these things when a pull request is opened.

Things we could start with immediately:

  • ESLint
  • TypeScript
  • Prettier
  • Tests

Which should block PRs from being merged.

And for the rest:

  • The Markdown spellcheck seems like a great idea.
  • JSHint seems to no longer be supported and has no recent activity, so I don't think I'd be in favor (unless there is something amazing we will miss)
  • On CSS linting, is there something here that Prettier can't do, or should we just enforce prettier rules on PRs?
  • Commitlint seems great, I would probably break that into a separate issue/discussion to argue on semantics. 😂

We could start with a warning (like a nice to have) until we have some firm docs and decisions.

@thanhsonng
Copy link
Contributor

I would love to explore this issue, starting with the items suggested as I want to learn more about GitHub actions (I also prefer the light commit approach)

@xiaoniuniu89
Copy link
Contributor

we should update when possible so builds can pass when deploying in vercel,

@NiallJoeMaher
Copy link
Contributor

@xiaoniuniu89 what updates are needed? 🤔 I'll get it added to the top of my pile.

@xiaoniuniu89
Copy link
Contributor

will be nice to prevent commit if lint fails, that was problem with my last pr.

@xiaoniuniu89
Copy link
Contributor

I caught it from running npm run build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants