-
Notifications
You must be signed in to change notification settings - Fork 9
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
Issue58 doc4 formatting add #60
Issue58 doc4 formatting add #60
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful to add the commands used to run these packages? I.e., npm run lint
and npm run format
.
I think so. I thought I put the formatting in though. Do you think it would be more clear to be explicit about each package in our |
I don't think every package needs to be explicitly explained, just those that affect the upload process. For example, I'm assuming testing and formatting will be part of the standard now. |
Got it. What about when we inform people that there's an update to remind them (if the Also - a few more changes have been done since you reviewed @RobinHerzig as there were updates to #57 |
The changes look good. I like the idea of telling people to run Do we have a quick reference for writing code and creating pull requests? If not, I can help with that. I just say that because, if people are meant to run |
We do not, though this idea is part of these:
One of the other members was going to take a stab at it from a prior meeting we had, but I don't know if they ever did anything with their notes. |
Examples might look like: | ||
|
||
``` | ||
npm install eslint --save-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think preferred is that they just run npm install
instead of installing things individually. Unless we're just trying to explain the concept here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was for concept yes, but we definitely don't want to cause confusion if we can avoid it - I'll look at updating this shortly
|
||
# Future Recommendations | ||
|
||
May want to consider integrating [eslint-config-prettier](https://github.com/prettier/eslint-config-prettier) or even [prettier-eslint](https://github.com/prettier/prettier-eslint). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done! I chose to go with just using the eslint extension here: https://github.com/codefordallas/codefordallas.github.io/blob/v1-mvp/.eslintrc.js#L2
This is great! Thanks for adding this Kassandra :) |
I can take a go at the PR template at least |
Fantastic! Thanks for taking a gander.
If you have the time and want to take a stab at it that would be great! Thank you @jhoover4 |
resolves #58 related to #57