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

Ayush/refactor #137

Merged
merged 18 commits into from
May 18, 2021
Merged

Ayush/refactor #137

merged 18 commits into from
May 18, 2021

Conversation

ayush-goyal
Copy link
Member

Changes

It might be easier to review the changes by commit, since the changes are organized better.
To merge after #136

  • Refactor some of the backend code into multiple files/folders
  • Switch Knex to Prisma and update all the queries and variable names and types
  • A lot of the changes on the frontend are simply naming changes
  • Switch npm to yarn (since prisma works weird with npm it was easier to do)

@ayush-goyal ayush-goyal requested a review from rahulrajus as a code owner April 22, 2021 05:19
@ayush-goyal ayush-goyal requested a review from evan10s April 22, 2021 05:20
Copy link
Member

@evan10s evan10s left a comment

Choose a reason for hiding this comment

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

Code changes look good but would like to see a functioning dev deployment to validate functionality

@hackgt-beekeeper hackgt-beekeeper bot temporarily deployed to bolt-dev May 16, 2021 23:35 Inactive
@hackgt-beekeeper hackgt-beekeeper bot temporarily deployed to bolt-dev May 16, 2021 23:40 Inactive
@hackgt-beekeeper hackgt-beekeeper bot temporarily deployed to bolt-dev May 17, 2021 00:12 Inactive
@hackgt-beekeeper hackgt-beekeeper bot temporarily deployed to bolt-dev May 17, 2021 00:36 Inactive
@ayush-goyal
Copy link
Member Author

ayush-goyal commented May 17, 2021

Ok I added a dev deployment at https://bolt.dev.hack.gt with Prisma. Check it out and let me know if everything is working for you. I can also make you an admin if needed. @evan10s

@evan10s
Copy link
Member

evan10s commented May 17, 2021

Cool, will check it out this week

@evan10s
Copy link
Member

evan10s commented May 18, 2021

I tested out the dev deployment. Just noticed one weird thing:

  • On the users page, changing a user's admin status causes the whole users table to flash
    • However, the user admin status change button is poorly implemented (read: I barely knew React at the time) so it could honestly just use a separate refactoring

I'm just going to make an issue for that (edit: #143); otherwise, this all seems fine to me.

@ayush-goyal
Copy link
Member Author

Cool! Will you go ahead and approve the PR?

@ayush-goyal ayush-goyal merged commit 6fa3afb into master May 18, 2021
@ayush-goyal ayush-goyal deleted the ayush/refactor branch May 18, 2021 17:12
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