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

#28 Implement standardised documentation and linting #29

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ryanolee
Copy link
Collaborator

@ryanolee ryanolee commented Nov 21, 2022

What

This PR adds a bit of documentation on how out hooks work and also staged-lint with prettier for automatically formatting code in the app

Why

So that our code styles are standard

How

By running linting on post commit

Covers

@wendyyng
Copy link
Contributor

  • I ran npm run lint and it used the prettier to automatically formatting the code - worked as expected
  • I then git commit on my local machine and lint-staged ran automatically to fix the formatting - also worked as expected.

@wendyyng
Copy link
Contributor

wendyyng commented Nov 22, 2022

@ryanolee I'm just wondering if you'd want to incorporate eslint as well? Based on the documentation, lint-staged can also run eslint but right now it seems like we are only using prettier. Please correct me if I'm wrong or missing something.

@ryanolee
Copy link
Collaborator Author

ryanolee commented Nov 22, 2022

Hi @wendyyng,
Thanks for looking at this!
I think we already used it as part of the CRA build process. Meaning ESLint warnings are displayed at build time 🤔 . Will double check if we can run it with the --fix argument as part of the staged-lint process 👀

@ryanolee
Copy link
Collaborator Author

Hi @wendyyng , I have added eslint to the post commit hook for this 👍

@ryanolee
Copy link
Collaborator Author

ryanolee commented Dec 5, 2022

Updated this to contain the latest changes 👍

Copy link
Collaborator

@LodenH16 LodenH16 left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just one change 🤘

@@ -9,7 +9,6 @@ admin.initializeApp();
const db = admin.firestore();

module.exports.onUserCreate = functions.auth.user().onCreate((user) => {
console.log(JSON.stringify(user))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch 👍

Comment on lines +1 to +9
# Scoped hooks

This folder contains a number of hooks relating to authencication and data fetching. This initially covers scoped hooks. Under the hood most interaction with firebased are covered by [react-firebase-hooks](https://www.npmjs.com/package/react-firebase-hooks).

# How they work

Most of the queries on in the MBO app require knowing the current user ID before queries can be performed. This is because the data model of the site follows a setup where all user specific data is stored under a `users/{USER_ID}/{COLLECTION_PATH}`. In order to simplify the process of waiting for this information to become available form firebase we create `userScoped` rendition of standard data fetching hooks which can be found [here](https://github.com/csfrequency/react-firebase-hooks/tree/09bf06b28c82b4c3c1beabb1b32a8007232ed045/storage)

Each of these hooks should return a shared loading state while proxying the API of the original hooks otherwise. Collection and Document paths are automatically prepended with `users/{USER_ID}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few typos and some clarification courtesy of Grammarly 😄

Suggested change
# Scoped hooks
This folder contains a number of hooks relating to authencication and data fetching. This initially covers scoped hooks. Under the hood most interaction with firebased are covered by [react-firebase-hooks](https://www.npmjs.com/package/react-firebase-hooks).
# How they work
Most of the queries on in the MBO app require knowing the current user ID before queries can be performed. This is because the data model of the site follows a setup where all user specific data is stored under a `users/{USER_ID}/{COLLECTION_PATH}`. In order to simplify the process of waiting for this information to become available form firebase we create `userScoped` rendition of standard data fetching hooks which can be found [here](https://github.com/csfrequency/react-firebase-hooks/tree/09bf06b28c82b4c3c1beabb1b32a8007232ed045/storage)
Each of these hooks should return a shared loading state while proxying the API of the original hooks otherwise. Collection and Document paths are automatically prepended with `users/{USER_ID}`
# Scoped hooks
This folder contains a number of hooks relating to authentication and data fetching. This initially covers scoped hooks. Under the hood, most interactions with Firebase are covered by [react-firebase-hooks](https://www.npmjs.com/package/react-firebase-hooks).
# How they work
Most of the queries in the MBO app require knowing the current user ID before queries can be performed. This is because the site's data model follows a setup where all user-specific data is stored under a `users/{USER_ID}/{COLLECTION_PATH}`. In order to simplify the process of waiting for this information to become available from firebase we create a `userScoped` rendition of standard data fetching hooks which can be found [here](https://github.com/csfrequency/react-firebase-hooks/tree/09bf06b28c82b4c3c1beabb1b32a8007232ed045/storage)
Each of these hooks should return a shared loading state while proxying the API of the original hooks otherwise. Collection and Document paths are automatically prepended with `users/{USER_ID}`

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