Skip to content
This repository has been archived by the owner on May 24, 2021. It is now read-only.

Adds limit & offset. Changes dataType to json on query request #38

Merged
merged 2 commits into from
Nov 5, 2014
Merged

Adds limit & offset. Changes dataType to json on query request #38

merged 2 commits into from
Nov 5, 2014

Conversation

ffflabs
Copy link
Contributor

@ffflabs ffflabs commented Nov 4, 2014

This PR adds a limit and offset parameters to the query method. While it might seem unnecesary, the idea is to comply with the current available properties of google.maps.FusionTablesQuery object.

Additionally, it changes the ajax request to googleapis.com/fusiontables to use dataType json instead of jsonp. The former is easier to debug in devTools, and googleapis.com has long implemented CORS so cross domain request won't be a problem. This allows for a cleaner callback section in which you can add more function calls or debug the response.

Instead of

MapsLib.query(selectColumns, whereClause, "", "", "MapsLib.displaySearchCount");

the call could be

MapsLib.query(selectColumns, whereClause, "", "", "", "", function(response) {
    console.info('Table response is',response);  
    MapsLib.displaySearchCount(response);
});

adds offset

adds offset

adds offset
@derekeder
Copy link
Owner

This is great!

Couple of things:

lengthy query() function call
Something that's always bugged me about the query() function is all the empty strings that are typically passed in. With these two additional fields, the call looks like this:

MapsLib.query(selectColumns, whereClause, "", "", "", "", function(response) {...});

What if we structured the function to take in an object with named parameters like so:

var query_opts = {
  'select': selectColumns,
  'where': whereClause
}

And take advantage of jQuery.extend so we only have to pass in the attributes that aren't blank. I think this would make working with it a little clearer. Thoughts?

updated docs
Agree that the dataType json approach is better. The List search results wiki example will need to be updated. This is mostly just a note for myself to do (along with #15).

@ffflabs
Copy link
Contributor Author

ffflabs commented Nov 5, 2014

You're right! I'll do that now.

(however, I don't quite see where do you want to use $.extend)

@derekeder
Copy link
Owner

Cool! I'll leave it up to you how to implement, but the idea goes something like:

MapsLib.query({ 'select': selectColumns, 'whereClause': whereClause }, function(response) {
    console.info('Table response is',response);  
    MapsLib.displaySearchCount(response);
});

...

query: function(options, callback) {

var default_options = {
  'selectColumns': '',
  'whereClause': '',
  'groupBy': '',
  'orderBy': '',
  'limit': '',
  'offset': ''
}

$.extend(default_options, options)
...
}

With this, you only pass in the attributes that aren't empty and the others are set to blank by default.

@ffflabs
Copy link
Contributor Author

ffflabs commented Nov 5, 2014

I see. In the last commit I implemented this, but instead of providing default values I check if the property exists and it's not an empty string. I believe it accomplishes the same 🐹

@derekeder
Copy link
Owner

@amenadiel yup you're right! This looks good - merging.

derekeder added a commit that referenced this pull request Nov 5, 2014
Adds limit & offset. Changes dataType to json on query request
@derekeder derekeder merged commit 7ab504a into derekeder:master Nov 5, 2014
@derekeder
Copy link
Owner

@ffflabs
Copy link
Contributor Author

ffflabs commented Nov 6, 2014

Seeing my PRs merged always makes my day 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants