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

Format all web code #436

Merged
merged 11 commits into from
Aug 16, 2023
Merged

Conversation

Pelsin
Copy link
Contributor

@Pelsin Pelsin commented Jul 30, 2023

No changes in code/functionality except the .editorconfig.
This is purely for developer experience, also want to make it clear this is just a suggestion on format according to prettier+editorconfig. Just wanted to normalize so we use the same formatting in all files.

This will help issues like what happened here: #417
Also thought it was a good idea to do it now as there are only a few PRs up touching the web part.

Ran prettier to format the files in the www folder.
npx prettier --write "{www/src,www/server}/**/*.{tsx,ts,jsx,js,json,scss}"

Changed .editorconfig:

  • handle EOL as lf, so it matches .gitattributes
  • js/ts files use single quotes

Added "format" npm script for those not running a prettier plugin.

npx prettier --write "{www/src,www/server}/**/*.{tsx,ts,jsx,js,json,scss}"
Copy link
Member

@SavageCore SavageCore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer singleQuote: true but otherwise, very nice and very welcome!

Should we add husky to enforce linting on commit?

lint script should be added to the package.json and we should then extend node.js.yml action to run lint tests.

@Pelsin
Copy link
Contributor Author

Pelsin commented Aug 7, 2023

I'd prefer singleQuote: true but otherwise, very nice and very welcome!

True, i also prefer single quotes. I've changing it in editorconfig for js/ts files. Will check with the others if someone objects :)

Should we add husky to enforce linting on commit?
lint script should be added to the package.json and we should then extend node.js.yml action to run lint tests.

Sounds like a great idea, had a short discussion with feralAi about the linting when we moved from CRA to vite.
There currently is a lint script in package.json, there are some errors that we wanted to clean up before making linting part of the pipeline.
Husky and adding to pipeline feels like a really nice next step, in another branch though!

Thanks for checking the PR out @SavageCore!

@SavageCore
Copy link
Member

@Pelsin I was going to suggest those could come later and I missed linting already being there silly me!

LGTM.

SavageCore
SavageCore previously approved these changes Aug 7, 2023
# Conflicts:
#	www/src/Pages/AddonsConfigPage.jsx
#	www/src/Pages/SettingsPage.jsx
Copy link
Contributor

@arntsonl arntsonl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome!

@arntsonl arntsonl merged commit b44df76 into OpenStickCommunity:main Aug 16, 2023
46 checks passed
@Pelsin Pelsin deleted the format-all-web-code branch September 25, 2024 17:35
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

Successfully merging this pull request may close these issues.

3 participants