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.

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

Live Search #47

Merged
merged 1 commit into from
Oct 16, 2017
Merged

Live Search #47

merged 1 commit into from
Oct 16, 2017

Conversation

vmjoseph
Copy link
Collaborator

@vmjoseph vmjoseph commented Oct 15, 2017 •
edited by blackjack26
Loading

  • Added participant live search
  • livesearch() can be called on any page (requires and HTML element to hold search results)
  • Included livesearch.js in header file

Fixes #15

@vmjoseph vmjoseph requested a review from blackjack26 October 15, 2017 20:16
blackjack26
blackjack26 previously requested changes Oct 15, 2017
View reviewed changes
public/js/livesearch.js Outdated
var identifier ;

//set values to vars, this is done wherever the livesearch is called
function setInputListener(inputField , resultField, resultHolder, uUrl, method, typeD){
Copy link
Collaborator

@blackjack26 blackjack26 Oct 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Add a JSDoc header to explain what the parameters are for in the method. Especially since this function will be used in other places than need a live search.

public/js/livesearch.js Outdated
@@ -0,0 +1,104 @@
//init vars
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment header to file (https://github.com/Capping-CPCA/Web-App/wiki/Adding-New-Pages#comment-header)

public/js/livesearch.js Outdated
}

//debugging
//console.log("live search is loaded");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this commented out code (unless you think you'll need it here later). Any place you have commented out debugging code I think should be removed to clean up the file.

public/js/livesearch.js Outdated
$(this).find(":text").val(filterResults($(this).find(":text").first().val()));
var userInput= $(this).find(":text").val();

//ajax - asynchronously requesting resutls from our result page
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type in comment: word resutls

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, didn't quite understand what you meant by this one

Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment has a typo in it haha

public/js/livesearch.js Outdated
//giving the form it's own var for simplicity sake, later functions redefine the 'this' method
var uForm = $(this);

//taking cleaning user supplied input - no extra paces or , will be queried
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you meant spaces instead of paces

views/agency-requests/live_search_test.php Outdated
@@ -0,0 +1,34 @@
<?php
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is your test file for live searches. Did you mean to remove this?

public/js/livesearch.js Outdated
return filtered;
}
//set the form input element text to cleaned text, nice and tidy
$(this).find(":text").val(filterResults($(this).find(":text").first().val()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

You created the uForm variable to replace having type $(this), should you use uForm here to replace the $(this)?

public/js/livesearch.js Outdated
//taking cleaning user supplied input - no extra paces or , will be queried
function filterResults(raw){
var filtered = raw.replace(/\s+/g,' ').trim().replace(',','');
return filtered;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can simplify this by making it one line:

return raw.replace(/\s+/g,' ').trim().replace(',','');

public/js/livesearch.js Outdated
//jquery for each short hand, populating result list with well...results
$(userResults).each(function(){
console.log($(this));
results.append(indiResults+ $(this).text()+ "</li>");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Appending the "</li>" to the end of the string assumes that everyone passed in an <li> element for resultHolder which may not be true. If it is always true then you might want to just remove the option to set the resultHolder and always make it the <li class='list-group-item suggestion'> element you passed in.

views/agency-requests/agency_requests.php Outdated
<button type="submit" class="btn cpca form-control">Submit</button>
<input type="text" class="form-control user-search" name="searchquery" placeholder="Begin typing participant's name...">
<ul class='list-group'>
</ul>
Copy link
Collaborator

Choose a reason for hiding this comment

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

When the list does drop down, I would suggest adding a hover effect (cursor -> pointer, background -> darker), so the user knows they can click on it.

@vmjoseph
Copy link
Collaborator Author

Alright I've updated my pull request with the request changes

@blackjack26
Copy link
Collaborator

Do you need the live_search_test.php file?

@vmjoseph
Copy link
Collaborator Author

Nope, removed anything important from it. It should have been removed?

@blackjack26
Copy link
Collaborator

It still seems to be in there if you look on the Files Changed tab.

@vjoseph2 vjoseph2 force-pushed the 15-live-search branch 2 times, most recently from 1388169 to ef5bc7d Compare October 16, 2017 00:18
@vmjoseph @blackjack26
live search currenty working, going to make it into a class for easie…
a3f20d2 
…r usability

live search now submits form on auto-complete click, need to test what happens when multiple participants have the same name

adjusting auto complete function

changing name of branch to match branch naming scheme

adding global function for live searching

leaving in db init file for now, fixing live search

Fixing lag issue that occurs during search

fixed submit bug on live search

finished live search feature, updated so that function is globally accessible

adding jsdoc comments and comment headers, removing debugging

simplifying filtered function

removing test file

updating references to form in livesearch function

removing reference to test search file

fixed issue with force list element option

fixing version number
Fixes #15

Added live search both to Agent Requests and for global use

added ui hover selection for search results

removed live search test
@blackjack26 blackjack26 changed the title 15 live search Live Search Oct 16, 2017
@vmjoseph vmjoseph merged commit 50e2e18 into develop Oct 16, 2017
@vmjoseph vmjoseph deleted the 15-live-search branch October 16, 2017 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blackjack26 blackjack26 blackjack26 approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vmjoseph @blackjack26