Open nmpereira opened 8 months ago
Thank you so much! I'm in school all week so it might be a bit before I can review and merge.
Tests are included in https://github.com/labrocadabro/communitytaught/pull/53
1) Is the filterTags.js necessary? I think it should be possible to build all URLs on the backend and just have them be regular clickable links, rather than building the URLs client-side and using buttons. You've got all the current tags in the query parameters when the page is visited, so it's just a matter of checking if each tag is already in the params. If no, you append it, otherwise you remove it. "Clear filters" can just be al link rather than a button and point back to /class/all.
2) Is the /class/filter endpoint needed? Not a big deal if it is, but again I think it should be possible to modify the existing /class/all page, and having two pages with essentially the same content can cause SEO issues.
3) I'd like the Filter section to be collapsed by default. The heading should probably be "filter by tags", we can remove the "available filters" text, and either the heading, tags, and results count should all be contained within a bordered box, or the border should be removed.
4) The redirecting back to a blank form rather than the same class was intentional. I'm the only one who uses it in production, and I spend more time adding classes in succession, rather than editing existing classes. (What it really should do is redirect to /add if the class is newly added and redirect to /edit if the class is being edited. I'll post a separate issue for that.) I'd appreciate it if you could revert that. In general, if you're making changes unrelated to the issue, I'd appreciate it if you could submit a separate issue/PR. This would also apply, for example, to the extra comments you added to .env.example. It's a very small change, but putting in an unrelated PR makes it less clearly documented. I'm fine with leaving it there for now because we don't have proper documentation for setting up the site for local development, but it really belongs in the docs rather than the .env as there should be a whole section on setting up OAuth.
TL;DR: Please make the tag feature fully server-side or just let me know why the client side code is necessary. 2 and 3 are optional. Please revert the change in lessons.js > addEditLesson() that redirects to /edit on submit -- I'll put up a separate issue for that. The comments in the .env file should have been a separate PR but don't bother as it's small, we'll just leave it here.
/class/all
, where the allLessons
controller would use the query and redirect the user? The allLessons
controller can either redirect or render, so I'm not sure how I could do both.From my perspective, the adding of the tags=
query should happen on the client side, but if you could elaborate a little more, I could look into changing it.
My thinking with /class/filter
was that, for one, you could use the filter by tag and the future search functionality in the /class/filter
route. I can do this on the /class/all
page but that would mean that the filtering component would show on the page at all times. I guess I could collapse this by default to keep it less distracting.
Will revert all the changes in 4 and make the changes in 3
closes #27
Added filtering by tags
/class/filter
which can be accessed by clicking on any tagImages
Tags on all classes page
Tags in each class
Edit tags
Select filters
Select multiple filters