search

Speedcubing-Canada / speedcubing-canada-web

https://speedcubingcanada.org
2 stars 3 forks source link

Backend with accounts and province ranking system #50

Open AlexandreOndet opened 11 months ago

AlexandreOndet commented 11 months ago

This PR is a new version #45, I made a new branch to showcase the new changes since the last code review, you can see them at : https://github.com/AlexandreOndet/speedcubing-canada-web/commit/db8642c2dbf7e43a3147cf1eab1984b271abcd0a

AlexandreOndet commented 11 months ago

Regarding the renaming of app dir to frontend I believe it's a good idea for the general organization of the repo, as we now have both the front and the back, it makes it much clearer and logical for someone who is not familiar with the repo. Also concerning the number of files changed, outside of the pdfs and images, most of them have been modified at one point or another so I'm not sure it would reduce by much the actual number of files that needs to be looked at. (as an example you can see here: https://github.com/AlexandreOndet/speedcubing-canada-web/tree/docker/frontend/src/pages that the About page is the only one that wasn't touched)

For the quebec discord feature, no problem I'll move the commit to another PR. The only problem is that on the branch the current PR is it's merged with many other commits so it will require a bit extra of extra work to revert it.

I agree with the external dependencies comment, I'll try removing axios in the next days. I don't know if you've noticed but I also managed to remove the dependency to WCA components and WCA helpers.

JonEsparaz commented 11 months ago

I'll be more firm about not renaming the app dir in this PR (I see why you want to rename it and you can rename it in a separate PR), however, doing it in this PR is simply bad software development and makes the review more difficult. Some files are presented as new files (with a deleted file) rather than modified files in the GitHub UI since the directory has been renamed and I'd rather not waste time sorting through what was and wasn't actually changed.

AlexandreOndet commented 11 months ago

Okay so the dir is renamed and I removed axios.

JonEsparaz commented 11 months ago

This PR will need to be rebased after merge of #52. I suspect the most challenging issue will be the package-lock.json conflicts. Since we've re-organized using NPM workspaces, we should have the following files:

./package.json
./package-lock.json
./app/package.json # note we don't have the lockfile in the /app directory any longer

The easiest way to fix any conflicts will be to run npm install at the project root and commit any changes in the root-level package-lock.json file. Your changes to ./app/package.json should still be valid.

kr-matthews commented 11 months ago

More admin page stuff: if I select 5 rows per page then it just shows all 13 rows.

AlexandreOndet commented 11 months ago

More admin page stuff: if I select 5 rows per page then it just shows all 13 rows.

I fixed it

AlexandreOndet commented 11 months ago

To answer the comments about the user list in the admin section. Currently are limited by outside factors. For pagination and searching, I will write an issue to describe that more precisely. (edit, done: #56 ) The "no users yet" is from not React admin, not from me so it's more complicated to change. And since it loads pretty fast I couldn't even see it on my side so I guess it's okay for now.

For the red part, I understand your concern but also it's doesn't shock me at all so I would like feedback from other people to see how they feel.