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

Add filters to homepage. #198

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

CalderWhite
Copy link
Contributor

@CalderWhite CalderWhite commented Dec 11, 2022

Notion ticket link

Home Page Filtering

Implementation description

  • Added existing filtering code to the homepage

Steps to test

  1. Attempt to select some filters and see if they are applied

Note: I had trouble testing this because the existing filtering options are empty (in /magazine/search_results). We should fix this as well.

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have run docker-compose up and my project compiled
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@github-actions
Copy link

github-actions bot commented Dec 11, 2022

Visit the preview URL for this PR (updated for commit 94d3bb1):

https://ccbc-95e66--pr198-calder-add-filters-t-ywoxtrvg.web.app

(expires Sun, 18 Dec 2022 20:48:50 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c378380023717371c81e2542a3b308a673eeb7f6

Copy link
Collaborator

@Shrinjay Shrinjay left a comment

Choose a reason for hiding this comment

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

I don't think this is going to work because we're not using the search url stuff for the homepage, and in general this is pretty messy... so I think my approach was dumb. My apologies. Instead get rid of the code from lines 81 to 98, and let's do this flow instead:

Essentially, when someone clicks on the filter, it should redirect them to the search page with that filter applied already, this is a facility we already have, so we just need to implement redirection:

Write handlers for setGenresFilter and setAgeRange filter that redirect to the search page with url params indicating the entered search state. Take inspiration from the generateSearchUrl funciton in searchReviews for this.


const setAgeRangeFilter = (ageRange) => {
// Get URL object
   const searchUrl = new URL(
        `${window.location.origin}/magazine/search_results/`,
      );
// Add filters to it
      searchUrl.searchParams.append("minAge", ageRange[0]
      searchUrl.searchParams.append("maxAge", ageRange[1])
// Push to react histroy (use the useHistory() hook in the component, look at SearchBox.tsx for example)
      history.push(searchUrl.toString());
}

Now implement this for the genres filter as well, and we should be good!

// filtering setup

// fetch filter params from URL
const params = new URLSearchParams(window.location.search);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is going to work because we're not using the search url stuff for the homepage, instead get rid of the code from lines 81 to 98

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.

2 participants