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

Fixes 2618 - Refactored search to use advanced route #3163

Closed

Conversation

AmasiaNalbandian
Copy link
Contributor

Issue This PR Addresses

Fixes #2618

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

In #2617 we added an advanced route. The advanced route is more flexible for search terms and capabilities. In this PR I changed the route used in the search results so that we use the new "advanced" route. In another PR I will clean up the regular search as we are now leveraging the more advanced route - and will be the main route meaning no more /advanced it will be /

Steps to test the PR

  • cp config/env.development .env
  • docker-compose --env-file ./config/env.development up --build search elasticsearch redis posts nginx planet
  1. Go to search and select author search an author - look in the network tab for dev tools, you will see the results coming from the new route.
  2. Do the same with posts.

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Mar 10, 2022

Copy link
Contributor

@RC-Lee RC-Lee left a comment

Choose a reason for hiding this comment

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

Is this PR needed? Cause I noticed you have SearchResults.tsx changed in #3164 as well

@AmasiaNalbandian
Copy link
Contributor Author

Is this PR needed? Cause I noticed you have SearchResults.tsx changed in #3164 as well

This is the smaller version of 3164.... This PR just changes the route to use, the other PR also ends up changing the route because we clean up all the other route and make advanced the main route.

@tpmai22 tpmai22 requested a review from RC-Lee March 11, 2022 00:11
@RC-Lee
Copy link
Contributor

RC-Lee commented Mar 11, 2022

Is this PR needed? Cause I noticed you have SearchResults.tsx changed in #3164 as well

This is the smaller version of 3164.... This PR just changes the route to use, the other PR also ends up changing the route because we clean up all the other route and make advanced the main route.

It still doesn't make sense to me why we can't do the change in just one PR.
I'd like the opinion of another reviewer.

@tpmai22
Copy link
Contributor

tpmai22 commented Mar 11, 2022

This seem arguable @Kevan-Y what do you think about this ?

@TueeNguyen TueeNguyen added this to the 2.8 Release milestone Mar 11, 2022
@AmasiaNalbandian
Copy link
Contributor Author

closing this - will just resolve in the other PR

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.

Refactor the search to use the advanced search route
4 participants