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

code style implementation #43

Merged
merged 6 commits into from
Nov 28, 2021
Merged

code style implementation #43

merged 6 commits into from
Nov 28, 2021

Conversation

decanTyme
Copy link
Contributor

@decanTyme decanTyme commented Nov 26, 2021

PR Includes:

  • Ignore .vscode folder
  • Add Prettier, config, ignore, scripts
  • Add ESLint, config, ignore, scripts
  • Add Yarn PnP VSCode integration
  • Update CONTRIBUTING.md
  • Add CI workflow
  • Some codestyle fixes

Summary:

Adds some checks and automations for anyone looking to contribute code (no testing yet.) The ESLint config extends the eslint-config-google. Overwritten rules are of my own personal preference.

Also, since Yarn PnP is used, some contributors that might be using VSCode with Prettier and ESLint extensions installed might encounter issues such as failing to find their respective modules so I generated some editor SDKs for integration with VSCode. These extensions should continue to work within this workspace. Basically, they fail to find a node_modules folder so they error out without this.

Lastly, I don't know much about this since I've only just recently migrated to Yarn [v1], but the .pnp.cjs file's packageLocation props have changed to my own dirs (e.g., from ./.yarn/cache/... to something like ../../../../../C:/Users/<USER>/AppData/Local/Yarn/Berry/cache/, is that what it's supposed to do?) since I did a yarn install. Won't start without running it so I had no choice.

p.s. Reminded me of those awesome ASCII rendering stuff, but this time with colors! So got a little excited and decided to help a little. Please do tell if there were some issues. Might add more PRs for new features/tests soon.

* Add Prettier, config, ignore, scripts
* Add ESLint, config, ignore, scripts
* Add CI workflow
* Update CONTRIBUTING.md
@vercel
Copy link

vercel bot commented Nov 26, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/warengonzaga/css-text-portrait-builder/5AGGcXz1AgLwUCmwp521JwiPxqbN
✅ Preview: https://css-text-portrait-builder-git-fork-decantyme-dev-warengonzaga.vercel.app

@decanTyme
Copy link
Contributor Author

Lastly, I don't know much about this since I've only just recently migrated to Yarn [v1], but the .pnp.cjs file's packageLocation props have changed to my own dirs (e.g., from ./.yarn/cache/... to something like ../../../../../C:/Users//AppData/Local/Yarn/Berry/cache/, is that what it's supposed to do?) since I did a yarn install. Won't start without running it so I had no choice.

I think I kinda get why this happened now. Did you happen to do a Yarn Zero-Install, @warengonzaga? In that case, I think you forgot to include the ./yarn/cache in the repo so when I ran yarn install, yarn add it failed to find that and redownloaded/regenerated everything:

The cache folder is by default stored within your project folder (in .yarn/cache). Just make sure you add it to your repository (see also, Offline Cache).

@warengonzaga
Copy link
Owner

Lastly, I don't know much about this since I've only just recently migrated to Yarn [v1], but the .pnp.cjs file's packageLocation props have changed to my own dirs (e.g., from ./.yarn/cache/... to something like ../../../../../C:/Users//AppData/Local/Yarn/Berry/cache/, is that what it's supposed to do?) since I did a yarn install. Won't start without running it so I had no choice.

I think I kinda get why this happened now. Did you happen to do a Yarn Zero-Install, @warengonzaga? In that case, I think you forgot to include the ./yarn/cache in the repo so when I ran yarn install, yarn add it failed to find that and redownloaded/regenerated everything:

The cache folder is by default stored within your project folder (in .yarn/cache). Just make sure you add it to your repository (see also, Offline Cache).

I'm trying to implement the Yarn's Zero Install but when I tried to apply gulp.js to the project it broke like hell. I guess the project is not ready for Zero Install (Plug n Play). I'm having an issue with that part.

@warengonzaga
Copy link
Owner

PR Includes:

  • Ignore .vscode folder
  • Add Prettier, config, ignore, scripts
  • Add ESLint, config, ignore, scripts
  • Add Yarn PnP VSCode integration
  • Update CONTRIBUTING.md
  • Add CI workflow
  • Some codestyle fixes

Summary:

Adds some checks and automations for anyone looking to contribute code (no testing yet.) The ESLint config extends the eslint-config-google. Overwritten rules are of my own personal preference.

Also, since Yarn PnP is used, some contributors that might be using VSCode with Prettier and ESLint extensions installed might encounter issues such as failing to find their respective modules so I generated some editor SDKs for integration with VSCode. These extensions should continue to work within this workspace. Basically, they fail to find a node_modules folder so they error out without this.

Lastly, I don't know much about this since I've only just recently migrated to Yarn [v1], but the .pnp.cjs file's packageLocation props have changed to my own dirs (e.g., from ./.yarn/cache/... to something like ../../../../../C:/Users/<USER>/AppData/Local/Yarn/Berry/cache/, is that what it's supposed to do?) since I did a yarn install. Won't start without running it so I had no choice.

p.s. Reminded me of those awesome ASCII rendering stuff, but this time with colors! So got a little excited and decided to help a little. Please do tell if there were some issues. Might add more PRs for new features/tests soon.

Huge thanks for the effort to improve the project, I really appreciate it. I'll review it now.

@warengonzaga warengonzaga added in review Issue/Pull Request Label for In Review Status tweak Issue/Pull Request for Tweaks labels Nov 27, 2021
@warengonzaga warengonzaga changed the title Add Prettier, ESLint, CI to enforce code style code style implementation Nov 27, 2021
Copy link
Owner

@warengonzaga warengonzaga left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, please review my requested changes. My workflow is mostly on single quotes and 4 spaces indents. I don't know what the standard is but from my recent popular projects we are using 2 space indent so I guess that's good for this also. My only concern is the single quotes implementation I guess you need to reconfigure the prettier I have config from the previous project I'll reference here so you can get an idea.

.gitignore Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/scss/main.scss Outdated Show resolved Hide resolved
@warengonzaga warengonzaga added waiting response Issue/Pull Request Label for Waiting Response and removed in review Issue/Pull Request Label for In Review Status labels Nov 27, 2021
@warengonzaga
Copy link
Owner

Hey, @decanTyme this is what we use from my previous project.
https://github.com/warengonzaga/gathertown.js/blob/main/.prettierrc.json

Please use this as a reference.

@warengonzaga warengonzaga added this to the v1.3.0 📦 milestone Nov 27, 2021
@warengonzaga warengonzaga linked an issue Nov 27, 2021 that may be closed by this pull request
@warengonzaga warengonzaga mentioned this pull request Nov 27, 2021
@decanTyme
Copy link
Contributor Author

decanTyme commented Nov 27, 2021

My workflow is mostly on single quotes and 4 spaces indents. I don't know what the standard is but from my recent popular projects we are using 2 space indent so I guess that's good for this also.

Yeah, 2 spaces are probably the way to go for any Node.js projects. Looks cleaner to me at least.

My only concern is the single quotes implementation I guess you need to reconfigure the prettier I have config from the previous project I'll reference here so you can get an idea.

Hey, @decanTyme this is what we use from my previous project. https://github.com/warengonzaga/gathertown.js/blob/main/.prettierrc.json

Please use this as a reference.

Got that. I'll also revert back to a .json ESLint config since I only made it into a .js because of line ending issues. Some checks fail when run in Ubuntu, for example, when developing in win32 and config is set incorrectly. Got really confused for some time why it fails. I didn't realize till now VSCode plays nicely with LF.

@warengonzaga
Copy link
Owner

Got that. I'll also revert back to a .json ESLint config since I only made it into a .js because of line ending issues. Some checks fail when run in Ubuntu, for example, when developing in win32 and config is set incorrectly. Got really confused for some time why it fails. I didn't realize till now VSCode plays nicely with LF.

Are you having issues right now?

@decanTyme
Copy link
Contributor Author

decanTyme commented Nov 27, 2021

Are you having issues right now?

None so far with the code. Though sometimes Yarn SDKs get cranky and I have to regenerate or extensions won't work (again.) There's also Parcel sometimes not detecting changes in the scss files and builds on whatever the last change was. Currently also working on something similar to #13, but for dynamically calculating maxChar since hardcoding it means it won't work for all screen sizes and orientations. Should this be a new issue? or should I give my thoughts in #13 directly?

I'm trying to implement the Yarn's Zero Install but when I tried to apply gulp.js to the project it broke like hell. I guess the project is not ready for Zero Install (Plug n Play). I'm having an issue with that part.

I guess I can take a look at that too. Exporting builds looks interesting and I might learn a thing or two before I fully migrate to Yarn.

@decanTyme
Copy link
Contributor Author

decanTyme commented Nov 27, 2021

@warengonzaga please take a look at the new changes (916b413), should have covered all your requests. Let me know if there's still issues. If none, I will mark this as ready and I guess all that's left after that is to properly configure Yarn's Zero Install starting with including ./.yarn/cache in the repo and reinstalling/regenerating deps as necessary.

@warengonzaga
Copy link
Owner

None so far with the code. Though sometimes Yarn SDKs get cranky and I have to regenerate or extensions won't work (again.) There's also Parcel sometimes not detecting changes in the scss files and builds on whatever the last change was. Currently also working on something similar to #13, but for dynamically calculating maxChar since hardcoding it means it won't work for all screen sizes and orientations. Should this be a new issue? or should I give my thoughts in #13 directly?

One more note on this, the variable $regular in _vars.scss can directly affect the maxChar variable at index.js. Meaning whenever you change the font size the max character we set will have a direct impact on the output. I'd like to use a monotype font in the future so we can predictively compute how many characters can fit on the screen.

For the image resize, I don't think it can directly affect the maxChar? Since the image is currently being used as background.

I guess I can take a look at that too. Exporting builds looks interesting and I might learn a thing or two before I fully migrate to Yarn.

The current build I guess is not Plug n Play I remember I reverted it back after I got errors. @decanTyme

@warengonzaga
Copy link
Owner

@warengonzaga please take a look at the new changes (916b413), should have covered all your requests. Let me know if there's still issues. If none, I will mark this as ready and I guess all that's left after that is to properly configure Yarn's Zero Install starting with including ./.yarn/cache in the repo and reinstalling/regenerating deps as necessary.

Awesome, I'll review it again.

@warengonzaga warengonzaga self-requested a review November 27, 2021 19:43
Copy link
Owner

@warengonzaga warengonzaga left a comment

Choose a reason for hiding this comment

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

For me, it is all good now.

@decanTyme
Copy link
Contributor Author

decanTyme commented Nov 28, 2021

One more note on this, the variable $regular in _vars.scss can directly affect the maxChar variable at index.js. Meaning whenever you change the font size the max character we set will have a direct impact on the output. I'd like to use a monotype font in the future so we can predictively compute how many characters can fit on the screen.

Yep, I factored this one too. Exporting scss variables is possible and with Parcel it couldn't have been any easier. I also factored in getting font information at runtime so this shouldn't be a problem. Resize events shouldn't be an issue either. Animation would then be possible after I complete this work.

The current build I guess is not Plug n Play I remember I reverted it back after I got errors. @decanTyme

Hmm. I guess it didn't roll back all the way, I still don't see any node_modules folder, or is Yarn v2+ supposed to be like that?

Anyhow, we'll leave those for another issue/pr I guess.

@decanTyme decanTyme marked this pull request as ready for review November 28, 2021 14:13
@warengonzaga
Copy link
Owner

Thanks, @decanTyme I'll merge this now!

@warengonzaga warengonzaga removed the waiting response Issue/Pull Request Label for Waiting Response label Nov 28, 2021
@warengonzaga warengonzaga merged commit 0b161d4 into warengonzaga:dev Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tweak Issue/Pull Request for Tweaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add CI/CD workflow
2 participants