-
Notifications
You must be signed in to change notification settings - Fork 5
Course Testimonials #46
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
base: master
Are you sure you want to change the base?
Course Testimonials #46
Conversation
…al + Notifications to users depending on testimonial result
…ual testimonial approvals
yutotakano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the awesome feature! There are several points I'd like to request changes or comments on, so I'd appreciate it if you could iterate on it when you have the time.
Major points are:
- I would prefer not to expose the category ID to the frontend. It's generally not good practice to expose raw DB IDs and slugs are perfectly equivalent for all the use cases of this PR
- Notification emails are broken as it currently stands
- Every user can currently see every testimonial, including rejected ones by someone else (in the network request)
- Some React and SQL optimizations and code style suggestions; a lot of the code feels very imperative (React is mostly suited for functional programming) and triggers unnecessary re-renders
- Mantine Datatable doesn't feel necessary
I'm aware there are lot of things I've left comments on... apologies! I'll be honest a lot of the code is currently a little messy and I wouldn't want that merged in just yet. It shouldn't be very hard to fix though!
If you don't have time because of your job etc, then don't worry; feel free to say and someone else (maybe me) can pick it up :)
| "@mantine/form": "^8.1.2", | ||
| "@mantine/hooks": "^8.0.0", | ||
| "@tabler/icons-react": "^3.0.0", | ||
| "@mantine/hooks": "^8.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The various Mantine package versions don't appear to be consistent with each other. This is bad, since Mantine packages are released in tandem and have compatibility issues if you combine multiple versions. If we're sticking to 8.2.1, everything should be 8.2.1.
But honestly, if we aren't relying on anything new, sticking with ^8.0.0 would be better. This is because the upstream repository (Community-Solutions at ETH) uses ^8.0.0 and it would make maintenance easier if they were aligned.
| "jwt-decode": "^4.0.0", | ||
| "katex": "^0.16.9", | ||
| "lodash-es": "^4.17.21", | ||
| "mantine-datatable": "^8.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% convinced that we need mantine-datatable for the testimonials list.
None of the columns except "number of testimonials" and "course name" really need sorting, so it's probably more lightweight to implement it on our own. It's not a particularly large and heavy table either, since there are only O(category_count) number of rows, not O(testimonial_count) rows. Normal Mantine table should be able to cope perfectly fine with such amount.
What do you think?
|
|
||
| </Modal> | ||
|
|
||
| <Card withBorder={true} radius="md" p={"lg"} style={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p="lg" is conflicting with "padding: 8px" right below. Please be consistent with the styling, and prefer the JSX props approach where possible. If complex inline styles are needed, I suggest moving them into CSS modules.
|
Thanks for the detailed review @yutotakano ! No worries at all, I'm happy to make the changes requested, and can slowly do it over the next week or two. |
|
Awesome!! If you have thoughts or disagree with any of my comments during development, let me know. Maybe your ideas are better :) |
Main Changes
Work in progress