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

Issue58 doc4 formatting add #60

Merged

Conversation

ProsperousHeart
Copy link
Member

resolves #58 related to #57

@ProsperousHeart ProsperousHeart marked this pull request as ready for review June 12, 2023 00:51
@ProsperousHeart ProsperousHeart added enhancement Not a requirement - enhances or improves project documentation Update to documentation requirements labels Jun 12, 2023
Copy link
Contributor

@RobinHerzig RobinHerzig left a 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.

@ProsperousHeart
Copy link
Member Author

ProsperousHeart commented Jun 12, 2023

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 package.json file?

@RobinHerzig
Copy link
Contributor

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.

@ProsperousHeart
Copy link
Member Author

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 package.json file was updated) to run npm install again?

Also - a few more changes have been done since you reviewed @RobinHerzig as there were updates to #57

@RobinHerzig
Copy link
Contributor

The changes look good. I like the idea of telling people to run npm install again after updating.

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 npm run lint and npm run format before committing code, then it'd be nice to have it a guide to follow when working in the terminal.

@ProsperousHeart
Copy link
Member Author

The changes look good. I like the idea of telling people to run npm install again after updating.

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 npm run lint and npm run format before committing code, then it'd be nice to have it a guide to follow when working in the terminal.

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.

@ProsperousHeart
Copy link
Member Author

will confirm with @jhoover4 once #49 is complete that this accurately reflects his code

Examples might look like:

```
npm install eslint --save-dev

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

Copy link
Member Author

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).

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

@jhoover4
Copy link

This is great! Thanks for adding this Kassandra :)

@jhoover4
Copy link

The changes look good. I like the idea of telling people to run npm install again after updating.
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 npm run lint and npm run format before committing code, then it'd be nice to have it a guide to follow when working in the terminal.

We do not, though this idea is part of these:

* [Create Repo Template for PRs #30](https://github.com/codefordallas/codefordallas.github.io/issues/30)

* [create template for issues #31](https://github.com/codefordallas/codefordallas.github.io/issues/31)

* [Create Guidelines For Contribution #32](https://github.com/codefordallas/codefordallas.github.io/issues/32)

* [Update Contribution File #51](https://github.com/codefordallas/codefordallas.github.io/issues/51)

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.

I can take a go at the PR template at least

@ProsperousHeart
Copy link
Member Author

This is great! Thanks for adding this Kassandra :)

Fantastic! Thanks for taking a gander.

The changes look good. I like the idea of telling people to run npm install again after updating.
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 npm run lint and npm run format before committing code, then it'd be nice to have it a guide to follow when working in the terminal.

We do not, though this idea is part of these:

* [Create Repo Template for PRs #30](https://github.com/codefordallas/codefordallas.github.io/issues/30)

* [create template for issues #31](https://github.com/codefordallas/codefordallas.github.io/issues/31)

* [Create Guidelines For Contribution #32](https://github.com/codefordallas/codefordallas.github.io/issues/32)

* [Update Contribution File #51](https://github.com/codefordallas/codefordallas.github.io/issues/51)

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.

I can take a go at the PR template at least

If you have the time and want to take a stab at it that would be great! Thank you @jhoover4

@ProsperousHeart ProsperousHeart merged commit c7b81cf into codefordallas:v1-mvp Oct 9, 2023
1 check passed
@ProsperousHeart ProsperousHeart deleted the issue58-Doc4FormattingAdd branch October 9, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Update to documentation enhancement Not a requirement - enhances or improves project requirements
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

DOC UPDATE: Explanation of Linting & Prettier
3 participants