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 the whole codebase #99

Merged
merged 9 commits into from
Apr 4, 2023
Merged

Format the whole codebase #99

merged 9 commits into from
Apr 4, 2023

Conversation

EricHasegawa
Copy link
Collaborator

🅱️ruh why did we not install eslint and do this earlier

@raineking
Copy link
Collaborator

raineking commented Apr 1, 2023

Fixed 90% of the eslint errors. The remaining deal with some api variable naming errors and wanted a second eye on them before changing the name (in case they need to be the same in the backend)

Screenshot 2023-04-01 at 1 37 10 AM

@EricHasegawa
Copy link
Collaborator Author

Fixed 90% of the eslint errors. The remaining deal with some api variable naming errors and wanted a second eye on them before changing the name (in case they need to be the same in the backend)

Screenshot 2023-04-01 at 1 37 10 AM

Yeah this was a false flag - disabled eslint for the line. Good catch!

@raineking
Copy link
Collaborator

One small thing before we merge, I'm getting these warnings when starting localhost. Firebase ones will probably be gone when we implement new auth but the last few can't find a file for the react-text-loop-next library. It is working fine on the site.

Screenshot 2023-04-03 at 2 56 36 AM

@EricHasegawa
Copy link
Collaborator Author

One small thing before we merge, I'm getting these warnings when starting localhost. Firebase ones will probably be gone when we implement new auth but the last few can't find a file for the react-text-loop-next library. It is working fine on the site.

Screenshot 2023-04-03 at 2 56 36 AM

should be fine, @jasont7 what do you think

@jasont7
Copy link
Member

jasont7 commented Apr 3, 2023

One small thing before we merge, I'm getting these warnings when starting localhost. Firebase ones will probably be gone when we implement new auth but the last few can't find a file for the react-text-loop-next library. It is working fine on the site.
Screenshot 2023-04-03 at 2 56 36 AM

should be fine, @jasont7 what do you think

shouldn't affect anything from what I can understand (might be worth looking into the react-text-loop more to see if it's behaving normally). this issue is talked about here.

Copy link
Member

@jasont7 jasont7 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 to me so far. just one comment - I see files that have 4-space tabs and others that have 2 spaces. shouldn't this be addressed in this pr?

@raineking
Copy link
Collaborator

looks pretty good to me so far. just one comment - I see files that have 4-space tabs and others that have 2 spaces. shouldn't this be addressed in this pr?

Oh yeah you're right, I think we have to add spaces configuration to eslint, I can add that quickly to fix

@EricHasegawa EricHasegawa merged commit d49544f into master Apr 4, 2023
@EricHasegawa EricHasegawa deleted the formatting branch April 4, 2023 12:55
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