-
Notifications
You must be signed in to change notification settings - Fork 24
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
WIP: WPRDC data fetching function #83
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.
This is fantastic. Exactly the kind of setup we need. I have some small changes, then we're good to merge
const WPRDC_QUERY_SUFFIX = '" '; | ||
|
||
// Fetch data from West Pennsylvania Regional Data Center using the SQL API | ||
function fetchWPRDCData(dataSourceName, options) { |
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.
FWIW, we can set default JS params in ES6, like:
function fetchWPRDCData(dataSourceName, options={}) {
So we don't have to have the conditional after. We're being selective in what es6 features we use, so this is fine as is. Just FYI.
options = {}; | ||
} | ||
|
||
console.group(`${dataSourceName} API`); |
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.
prune out all logging
|
||
records.forEach((record, i) => { | ||
// TODO: Check browser compatability for `instanceof Function` | ||
if (dataSource.processRecord instanceof Function) { |
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.
I think we can do without the instanceof
check, and just check for the existence of the processRecord
field. I can't think of a scenario when it wouldn't be a function
record.pin.addTo(map) | ||
markers.push(record); | ||
} else { | ||
console.log('Found a null location!', record); |
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.
We'll fix this with #61
const latLongNoNulls = latLong.some((field) => !!field); | ||
if (latLongNoNulls) { | ||
let title = null; | ||
if (dataSource.title instanceof Function) { |
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.
Let's assume that all datasets have a title and that it is a function.In fact, could you add this for the police and 311? It can be the field that you're using now for the display.
icon: dataSource.icon | ||
}); | ||
|
||
if (dataSource.popup instanceof Function) { |
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.
Again, with #79 we can assume that all datasets will have a function to generate the popup that can , so we don't have to have this check, I think.
html: '<i class="fa fa-commenting"></i>', | ||
iconSize: [32, 32], | ||
iconAnchor: [16, 32] | ||
Promise.all([ |
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.
Wow, this looks so clean
I corrected everything but part of the |
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.
Looks awesome! This is going to be a great help
This is a work in progress. Please comment, suggest changes, and criticisms.
This is for #17.
The idea is instead of duplicating the
fetch()
code for every request to WPRDC that a function makes all the calls and does the error handling (to be implemented).