Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[fix bug 763528] Implement AJAX search in /people page. #195

Merged
merged 1 commit into from Jun 20, 2012

Conversation

Projects
None yet
5 participants
Member

glogiotatidis commented Jun 18, 2012

No description provided.

@ednapiranha ednapiranha and 1 other commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
@@ -22,9 +21,7 @@ function initialize_map() {
if (val != '') {
search_string = '';
$('input#searchfield').val(search_string);
- qs.search(search_string);
- qs.cache();
- redraw_grid();
+ $('input#searchfield').trigger('input');
@ednapiranha

ednapiranha Jun 18, 2012

Member

do you have more than one id named #searchfield? if not, you can just simplify and call $('#searchfield') instead

@glogiotatidis

glogiotatidis Jun 19, 2012

Member

On Mon, 2012-06-18 at 10:50 -0700, Edna Piranha wrote:

@@ -22,9 +21,7 @@ function initialize_map() {
if (val != '') {
search_string = '';
$('input#searchfield').val(search_string);

  •        qs.search(search_string);
    
  •        qs.cache();
    
  •        redraw_grid();
    
  •        $('input#searchfield').trigger('input');
    

do you have more than one id named #searchfield? if not, you can just simplify and call $('#searchfield') instead


Reply to this email directly or view it on GitHub:
https://github.com/mozilla/remo/pull/195/files#r1003627

ok!

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
$('#profiles-number-of-reps').html(number_of_reps);
+ if (number_of_reps == 1)

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
$('#profiles-number-of-reps').html(number_of_reps);
+ if (number_of_reps == 1)
+ $('#profiles-number-of-reps-plural').html('');
+ else
+ $('#profiles-number-of-reps-plural').html('s');
+}
+
+var update_results = function(query) {
+ return function(data) {
+ console.log('Updating results');
@ednapiranha

ednapiranha Jun 18, 2012

Member

do you need this in here for production?

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
$('#profiles-number-of-reps').html(number_of_reps);
+ if (number_of_reps == 1)
+ $('#profiles-number-of-reps-plural').html('');
+ else
+ $('#profiles-number-of-reps-plural').html('s');
+}
+
+var update_results = function(query) {
+ return function(data) {
+ console.log('Updating results');
+ $('#search-icon').html('s');
+
+ if ($(location).attr('hash').substring(2) != query) {

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
$('#profiles-number-of-reps').html(number_of_reps);
+ if (number_of_reps == 1)
+ $('#profiles-number-of-reps-plural').html('');
+ else
+ $('#profiles-number-of-reps-plural').html('s');
+}
+
+var update_results = function(query) {
+ return function(data) {
+ console.log('Updating results');
+ $('#search-icon').html('s');
+
+ if ($(location).attr('hash').substring(2) != query) {
+ console.log('Values different', $(location).attr('hash').substring(2), query);
@ednapiranha

ednapiranha Jun 18, 2012

Member

also.. need for prod?

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
+
+ clear_map();
+ $('#grid-search-list').empty();
+ $('#table-search-list').empty();
+ $('#gridItem').tmpl(data.objects).appendTo('#grid-search-list');
+ redraw_grid();
+ set_number_of_reps(data.meta.total_count);
+ $('#listItem').tmpl(data.objects).appendTo('#table-search-list');
+ $('input#searchfield').data('searching', undefined);
+ add_pointers();
+ }
+}
+
+function request_timeout() {
+ // Unset data-searching after half a second to deal with API timeouts.
+ console.log('Timeout');

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
+ // Unset data-searching after half a second to deal with API timeouts.
+ console.log('Timeout');
+ $('input#searchfield').data('searching', undefined);
+ $('#search-icon').html('s');
+}
+
+function set_dropdown_value(name, value) {
+ $(name).val(value);
+ // We have to force trigger 'change' for foundation to update.
+ $(name).trigger('change');
+}
+
+
+function send_query() {
+ value = $(location).attr('hash').substring(2);
+ console.log('Value', value);

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
+ $('#search-icon').html('s');
+}
+
+function set_dropdown_value(name, value) {
+ $(name).val(value);
+ // We have to force trigger 'change' for foundation to update.
+ $(name).trigger('change');
+}
+
+
+function send_query() {
+ value = $(location).attr('hash').substring(2);
+ console.log('Value', value);
+
+ // Make sure we are not firing the same same request twice.
+ if ($('input#searchfield').data('searching') == value) {

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
+
+
+function send_query() {
+ value = $(location).attr('hash').substring(2);
+ console.log('Value', value);
+
+ // Make sure we are not firing the same same request twice.
+ if ($('input#searchfield').data('searching') == value) {
+ return;
+ }
+ $('input#searchfield').data('searching', value)
+
+ // Unbind change events to avoid triggering twice the same action.
+ unbind_events();
+
+ console.log('Sending', value);
@ednapiranha

ednapiranha Jun 18, 2012

Member

ahem :) anyway, you get the idea.. just find these and remove them

@ednapiranha ednapiranha and 4 others commented on an outdated diff Jun 18, 2012

media/js/profiles_people.js
+ if ($('input#searchfield').data('searching') == value) {
+ return;
+ }
+ $('input#searchfield').data('searching', value)
+
+ // Unbind change events to avoid triggering twice the same action.
+ unbind_events();
+
+ console.log('Sending', value);
+
+ // Form query based on URL
+ extra_q = ''
+ country = hash_get_value('country');
+ set_dropdown_value('#adv-search-country', country);
+ if (country)
+ extra_q += '&profile__country__iexact=' + country;
@ednapiranha

ednapiranha Jun 18, 2012

Member

wat? i dont think this is right - you shouldn't be passing django-specific stuff in javascript. can you rewrite this so that the django stuff is in the controller/forms.py section and pass variables in via ajax?

@glogiotatidis

glogiotatidis Jun 19, 2012

Member

It's not that bad as it seems! :D

Actually this is the django-tastypie way We are not directly talking to the db, it's just the syntax which is the same ;)

http://django-tastypie.readthedocs.org/en/latest/resources.html#basic-filtering

@rlr

rlr Jun 19, 2012

Member

no me gusta.

@ppapadeas

ppapadeas Jun 19, 2012

Contributor

εμένα μου αρέσει.

@rlr

rlr Jun 19, 2012

Member

haha, it is cool and all. I just think it is kind of tightly coupling the front end code to the backend structure. But if that is the way tasty pie is recommending, so be it. My experience with tasty pie has been with pretty basic queries so far.

You get to maintain it, your call :-)

@glogiotatidis

glogiotatidis Jun 19, 2012

Member

yo ricky, care to elaborate?

@ednapiranha

ednapiranha Jun 19, 2012

Member

so here's the deal: what you're doing is not incorrect but it's too tightly coupled to django-tastypie. if you think about it in terms of keeping the clientside js modular and reusable, then you could never move this js to another framework (assuming you ported it to something else that didn't use tastypie).

if you plan to add more api-level functionality to this codebase, then either you have to start organizing the js side to be more structured (e.g. backbone, ember, shipyard, etc) or you need to abstract the tastypie stuff as a shim in the backend. everything passed from the clientside should be generic and not tastypie-specific.

again, this is just a recommendation which i think will save you pain in future maintenance :) but if you decide not to do it, then keep it this way, adding to it in future changes will give you more problems.

@rlr

rlr Jun 19, 2012

Member

I am +1 on using a framework (I personally have been using backbone). But I realize that might be a bit of work for you to convert this now and you probably have to ship. Something to think about next time around. And if this starts getting way out of hand, then you can think about converting.

Also, sorry for landing here at random and being negative with "no me gusta." :-)

@glogiotatidis

glogiotatidis Jun 19, 2012

Member

I see what you mean. I'm going to have an API call on /people page and one /events page. So far we're not planning any other front-end to back-end interaction through API so I prefer to keep JS frameworks out of the picture for now. I'll definitely revisit this decision if we end up with more js.

thanks!

@pmclanahan

pmclanahan Jun 20, 2012

Owner

I'm with edna and rlr on this. I wish it were less tightly coupled, but it's good for getting things done now, especially if there aren't grand plans for adding a lot more of this kind of thing. Also tastypie++.

ship it!

@glogiotatidis

glogiotatidis Jun 21, 2012

Member

ACK. Btw since I've never worked with such a framework before what do you suggest? I see rlr works with backbone, @ednapiranha @pmclanahan ?

@ednapiranha

ednapiranha Jun 21, 2012

Member

depends on how structured you want it - if you use backbone it's more of a minimal mvc.. otherwise you can use ember or shipyard

@glogiotatidis

glogiotatidis Jun 21, 2012

Member

Aha got it. thnx!

@ednapiranha ednapiranha commented on an outdated diff Jun 18, 2012

remo/base/templates/base.html
@@ -275,7 +275,12 @@
<!-- Included JS Files -->
<script src="https://browserid.org/include.js" type="text/javascript"></script>
<script src="//www.mozilla.org/tabzilla/media/js/tabzilla.js"></script>
- {{ js('common') }}
+ {% if get_development_environent() %}
@ednapiranha

ednapiranha Jun 18, 2012

Member

typo? i think you meant environment?

@ednapiranha ednapiranha and 1 other commented on an outdated diff Jun 19, 2012

media/js/profiles_people.js
+
+ // Unbind change events to avoid triggering twice the same action.
+ unbind_events();
+
+ // console.log('Sending', value);
+
+ // Form query based on URL
+ extra_q = ''
+ country = hash_get_value('country');
+ set_dropdown_value('#adv-search-country', country);
+ if (country)
+ extra_q += '&profile__country__iexact=' + country;
+
+ area = hash_get_value('area');
+ set_dropdown_value('#adv-search-area', area);
+ if (area)
@ednapiranha

ednapiranha Jun 19, 2012

Member

a little nit :) if you could be so kind as to adding curly braces around your if statements that would be awesome :)

@ednapiranha ednapiranha commented on the diff Jun 19, 2012

media/js/profiles_people.js
+ }
+
+ $.ajax({
+ url: '/api/v1/rep/?limit=0&order_by=profile__country,last_name,first_name' + extra_q,
+ success: update_results(value),
+ error: request_timeout,
+ timeout: 1000
+ });
+
+ // Rebind events.
+ bind_events();
+}
+
+function hash_set_value(key, value) {
+ // Set value for key in hash
+ hash = $(location).attr('hash').substring(2).toLowerCase().replace(/\/$/, '');
@ednapiranha

ednapiranha Jun 19, 2012

Member

apparntly in some IE browsers, this may not render expected results as it will return http://blah.com#test and not #test. can you try testing it in versions of IE that this app is being accessed by? you could always just do something like $(location).split('#')[1]

@glogiotatidis

glogiotatidis Jun 20, 2012

Member

Just tried that with IE8 and seems to work. I guess jquery may provide some cross browser workarounds?

The search doesn't seem to work as expected though for other reasons which i'm still investigating. I'll reply here with findings, meanwhile please don't merge

@glogiotatidis

glogiotatidis Jun 20, 2012

Member

btw do you know any good services that i can check different browser functionality, and not just rendering?

@ednapiranha

ednapiranha Jun 20, 2012

Member

a vm to windows? :) haha that's what i usually do.

@ednapiranha ednapiranha pushed a commit that referenced this pull request Jun 20, 2012

Edna Piranha Merge pull request #195 from glogiotatidis/763528-replace-javascript-…
…instant

[fix bug 763528] Implement AJAX search in /people page.
746811e

@ednapiranha ednapiranha merged commit 746811e into mozilla:master Jun 20, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment