-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add an instant post search #613
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
Conversation
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.
It looks great!
* @Route("/search", name="blog_search") | ||
* @Method("GET") | ||
* | ||
* @return JsonResponse |
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.
Should be @return Response|JsonResponse
.
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.
JsonResponse
inherits from Response
so JsonResponse
should be enough.
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.
@yceruto, the controller returns a Response
instance when a request is not an XMLHttpRequest
, so I think JsonResponse
is not enough for this case.
assets/scss/app.scss
Outdated
@@ -67,6 +67,20 @@ header .locales a { | |||
padding-right: 10px | |||
} | |||
|
|||
header .search-bar { |
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.
All this code is not used now, so it should be removed.
@@ -59,4 +59,51 @@ private function createPaginator(Query $query, $page) | |||
|
|||
return $paginator; | |||
} | |||
|
|||
public function findBySearchQuery($rawQuery, $limit = Post::NUM_ITEMS) |
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.
Missing docblock
->getResult(); | ||
} | ||
|
||
private function sanitizeSearchQuery($query) |
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.
Missing docblock
]; | ||
} | ||
|
||
return new JsonResponse($results); |
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.
return $this->json($results)
?
|
||
<div id="results"> | ||
</div> | ||
{% endblock %} |
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.
@javiereguiluz, you forgot to add a widget with the Show source
button.
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.
@javiereguiluz, It seems that you forgot to commit the build/js/search.js
file.
} | ||
|
||
/** | ||
* Removes all non-alphanumeric characters except whitespaces |
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.
Missing dot at the end.
|
||
/** | ||
* @param string $rawQuery The search query as input by the user | ||
* @param int $limit The maximum number of results returned |
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.
Incorrect indent.
@voronkovich thanks for contributing this feature and thanks to reviewers too! |
This PR was merged into the master branch. Discussion ---------- Add an instant post search I've taken the code commitedd by @voronkovich in #173 and made some changes to it. Let's see if you like it. I propose to implement it like Google search results instead of displaying it in the main menu:  The reason is that the main menu is crowded when the user is logged in and the search bar looks bad. Commits ------- fe2e2ab Fixed CS issues 51ab96a Added missing search.js file f219db9 Changes requested by reviewers 0c236ed Refactored the feature da43022 Fix cs issues f45c31c Add an instant post search
I've taken the code commitedd by @voronkovich in #173 and made some changes to it. Let's see if you like it. I propose to implement it like Google search results instead of displaying it in the main menu:
The reason is that the main menu is crowded when the user is logged in and the search bar looks bad.