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

feat/video tags #27 #51

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

nmpereira
Copy link
Contributor

@nmpereira nmpereira commented Jan 8, 2024

closes #27

Added filtering by tags

  • Tags each class on all classes page
  • Tags on each class on individual class page
  • Tags editor on the edit class page (admin only)
  • Created an endpoint /class/filter which can be accessed by clicking on any tag
  • Created new "filter page" with a filter picker component to select/unselect single/multiple tags

Images

Tags on all classes page

image

Tags in each class

image

Edit tags

image

Select filters

image

Select multiple filters

image

@@ -25,5 +25,6 @@ GOOGLE_ID = x
GOOGLE_SECRET = x

# Only needed if you are using Github login
# For local development, use http://localhost:3000 as the homepage URL and http://localhost:3000/oauth/github/callback as the callback URL
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little bit of extra documentation to help with project setup

message: [`Class not ${!!req.params.id ? "updated" : "added"}`],
};
} finally {
res.redirect(`/class/edit/${req.params.id}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When submitting an "edit class" form, it redirected to a new blank form, this redirects back to the same class

@@ -85,6 +85,7 @@
@apply text-right;
}
#timestamps,
#tags,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should i be checking in src/assets/css/index.css?

@nmpereira nmpereira marked this pull request as ready for review January 8, 2024 03:49
@labrocadabro
Copy link
Owner

Thank you so much! I'm in school all week so it might be a bit before I can review and merge.

@nmpereira nmpereira mentioned this pull request Jan 12, 2024
@nmpereira
Copy link
Contributor Author

Tests are included in #53

@labrocadabro
Copy link
Owner

  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.

@nmpereira
Copy link
Contributor Author

nmpereira commented Jan 15, 2024

  1. If its not client side js, then I'm not quite clear on how it would work for building the url on the controller. From what i understand, do you mean, when a user clicks on a link, they go to /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.

  1. 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

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.

feature request: video tags / grouping
2 participants