Skip to content

Add an instant search (via AJAX) #173

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 27, 2017
Merged

Add an instant search (via AJAX) #173

merged 2 commits into from
Jul 27, 2017

Conversation

voronkovich
Copy link
Contributor

It's almost like the Google... but better
img1
img2
img3
img4

@bocharsky-bw
Copy link
Contributor

Wow, it's cool! But I think it's a bit overhead for demo project.

@javiereguiluz
Copy link
Member

@voronkovich 👍 I like this idea a lot. Thanks for working on it.

However, after a first quick review of the code, I find the TermSplitter and the TermEmphasizer features a bit overkill for the demo project. Please don't make any changes yet, but I think we should get rid of both of them and focus just on the Ajax feature.

@voronkovich
Copy link
Contributor Author

@javiereguiluz, if we remove the TermSplitter we will have to validate the ajax request anyway. The TermSplitter just only remove invalid characters from the query string and splits it into words.
I agree with removing TermEmphasizer. It is overhead, you're right.

@voronkovich
Copy link
Contributor Author

I've refactored the code. All overheads were beheaded.
overhead

@voronkovich voronkovich force-pushed the instant-search branch 2 times, most recently from 7865f32 to 0f183f4 Compare September 5, 2015 20:05
theofidry
theofidry reviewed Sep 5, 2015
View reviewed changes
src/AppBundle/Controller/BlogController.php Outdated
*/
public function searchAction(Request $request)
{
$q = $request->query->get('q');
Copy link

Choose a reason for hiding this comment

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

let's avoid a 1 char variable name :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's vastly using name for a search request. The Symfony search uses this name http://api.symfony.com/2.7/index.html?q=request

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, variable's name is matching with input name, so I prefer to use $q to be consistent.

Copy link

Choose a reason for hiding this comment

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

yeah, as an URL param, not a PHP variable

@voronkovich
Copy link
Contributor Author

I've renamed the $q variable to $query and removed the $em, because it's not needed here

stof
stof reviewed Sep 7, 2015
View reviewed changes
src/AppBundle/Controller/BlogController.php Outdated
return 2 <= strlen($term);
});

$posts = new ArrayCollection();
Copy link
Member

Choose a reason for hiding this comment

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

this should be an array. Doctrine does not return collection objects when doing queries

@voronkovich
Copy link
Contributor Author

Fixed. Thanks @stof!

javiereguiluz
javiereguiluz reviewed Sep 9, 2015
View reviewed changes
app/Resources/assets/js/jquery.instantSearch.js Outdated
return;
}

addItemToPreview(item)
Copy link
Member

Choose a reason for hiding this comment

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

do we need a semicolon here to avoid issues in some browsers?

Copy link
Member

Choose a reason for hiding this comment

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

there is no issue here. Browsers are consistent in the way they insert semi-colons

Copy link
Member

Choose a reason for hiding this comment

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

OK. Then I ask for this change because of our own code consistency. Either add this semicolon ... or remove all the other semicolons. Thanks!

stof
stof reviewed Sep 9, 2015
View reviewed changes
app/Resources/assets/js/jquery.instantSearch.js Outdated
};

var throttle = (function(){
var timer = 0;
Copy link
Member

Choose a reason for hiding this comment

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

your throttling function is flawed: it can be used only once as all usages of the throttle function are sharing the same timer

@voronkovich
Copy link
Contributor Author

Fixed again. Thanks, @stof!

@xabbuh
Copy link
Member

xabbuh commented Sep 11, 2015

@voronkovich Seems like this needs a rebase.

@voronkovich
Copy link
Contributor Author

@xabbuh can you shortly explain what exactly I have to do with it?

@stof
Copy link
Member

stof commented Sep 11, 2015

See http://symfony.com/doc/current/contributing/code/patches.html#rework-your-patch

@stof
Copy link
Member

stof commented Sep 18, 2015

-1 on this implementation for now. We should not advocate strip_tags as a way to produce safe HTML strings, as it is not its behavior (and even less its goal)

@javiereguiluz
Copy link
Member

OK then. Let's wait a bit to solve the XSS issues :)

@voronkovich voronkovich force-pushed the instant-search branch 3 times, most recently from 7a32167 to a6a3192 Compare September 18, 2015 15:21
@voronkovich
Copy link
Contributor Author

Fixed! Thanks, @stof!

@voronkovich voronkovich force-pushed the instant-search branch 3 times, most recently from f7098dc to c651e44 Compare October 31, 2015 18:32
@voronkovich
Copy link
Contributor Author

@javiereguiluz, what about to merge this PR?

@javiereguiluz javiereguiluz mentioned this pull request Dec 28, 2015
Closed
@voronkovich voronkovich force-pushed the instant-search branch 3 times, most recently from 6f700c1 to d187dcd Compare December 28, 2015 22:23
@voronkovich voronkovich force-pushed the instant-search branch 5 times, most recently from 176b753 to 3194067 Compare March 1, 2017 17:06
@javiereguiluz javiereguiluz mentioned this pull request Jul 26, 2017
Merged
@javiereguiluz javiereguiluz merged commit da43022 into symfony:master Jul 27, 2017
javiereguiluz added a commit that referenced this pull request Jul 27, 2017
@javiereguiluz
feature #613 Add an instant post search (voronkovich, javiereguiluz)
0d1ddaf 
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:

![search-results](https://user-images.githubusercontent.com/73419/28628630-783fd8e4-7225-11e7-89f3-c5941e92a4df.gif)

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
@javiereguiluz javiereguiluz mentioned this pull request Jul 27, 2017
Closed
Koc
Koc reviewed Jul 27, 2017
View reviewed changes
src/AppBundle/Controller/BlogController.php
$results = [];

foreach ($posts as $post) {
array_push($results, [
Copy link

Choose a reason for hiding this comment

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

why not just $results[] = xxx ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Koc, I don't know :) Anyway, this code was changed in #613.

Copy link
Member

Choose a reason for hiding this comment

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

We finally used $results[] = xxx ... mostly because PHPStorm displayed a pretty annoying message suggesting that 😄

@voronkovich voronkovich deleted the instant-search branch July 27, 2017 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@Koc Koc Koc left review comments

Assignees
No one assigned
Labels
Feature FINISHED
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@voronkovich @bocharsky-bw @javiereguiluz @xabbuh @stof @Koc @theofidry