-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Load data and kepler.gl file using URLs #260
Conversation
package.json
Outdated
"cover": "nyc --reporter html --reporter cobertura --reporter json-summary npm test && nyc report", | ||
"start": "npm run install-and-start -- examples/demo-app start-local", | ||
"start:": "npm run install-and-start -- examples/open-modal start-local", |
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.
2 start scripts?
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.
hmm, both scripts were already present in package.json. It was added in this commit: 25e18b0
I placed them close to each other to be more consistent.
yarn.lock
Outdated
@@ -5,6 +5,7 @@ | |||
"@deck.gl/core@^5.3.3": | |||
version "5.3.3" | |||
resolved "https://registry.yarnpkg.com/@deck.gl/core/-/core-5.3.3.tgz#a13c07e5fa3e22297fd450d6da8ab9aac334b1f0" | |||
integrity sha512-1a4me1QRM3/wzEVzZRAflX/Uh67gE9/3fWjl35iVY7lvNrCP0wl2PUfaQkmeThTa4Sp/Qq24ZTpHx1QvkQYH8g== |
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.
remove this integrity entries
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.
these entries are generated by the new yarn. I will revert them
examples/demo-app/src/actions.js
Outdated
// The following methods are only used to load SAMPLES | ||
/** | ||
* | ||
* @param 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.
not valid JSDoc
a707c66
to
07a5f96
Compare
- Refactored the way we load samples using promises
7312b7d
to
cd92db5
Compare
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.
LGTM, Some minor comments, maybe add a TODO to cover them later
examples/demo-app/src/actions.js
Outdated
@@ -111,21 +242,19 @@ function loadRemoteMap(sample) { | |||
* @returns {function(*)} | |||
*/ | |||
export function loadSampleConfigurations(sampleMapId = null) { | |||
return (dispatch) => { | |||
return dispatch => { | |||
requestJson(MAP_CONFIG_URL, (error, samples) => { | |||
if (error) { | |||
Console.warn(`Error loading sample configuration file ${MAP_CONFIG_URL}`); |
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.
shouldn't this error also be caught and displayed in the UI?
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 need to update the sample UI in order to show the error. I will do that
examples/demo-app/src/actions.js
Outdated
return new Promise((resolve, reject) => { | ||
requestMethod(url, (error, result) => { | ||
// TODO: we need to let users know about CORS issues | ||
error ? reject(error) : resolve(result) |
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.
Error will not be presented in all failed request, statusCode 304 for example will not send an error back.
An example of over engineered (but catch all) error handling can be found here:
https://code.uberinternal.com/diffusion/VIVOY/browse/master/src/shared/api/request-utils.js$30-37
<InputForm> | ||
<StyledDescription>Load your map using your custom URL</StyledDescription> | ||
<StyledInputLabel> | ||
You can use the following formats: CSV | JSON | KEPLERGL.JSON. Make sure the url contains the file extension. |
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.
is KEPLERGL.JSON a required file extension to load the config json? if not, just mention, the json format can also be a kepler.gl config json. Otherwise it's a little confusing
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.
KEPLERGL.JSON is not required. I only used it because that's the file name we generate with kepler.
I am going to update the content of the description
examples/demo-app/src/actions.js
Outdated
* @param config | ||
*/ | ||
function loadMapCallback(dispatch, error, result, options, config) { | ||
if (error) { |
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.
you can return a dispatch => {}
function, instead of passing the diaptch around
return dispatch => {
if (error) {...}
else {
dispatch(...)
}
}
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 will review this one. I think before this diff we were using loadMapCallback in multiple locations but now we may not need to encapsulate it anymore
examples/demo-app/src/actions.js
Outdated
*/ | ||
function loadMapCallback(dispatch, error, result, options, config) { | ||
if (error) { | ||
Console.warn(`Error loading datafile ${options.dataUrl}`); |
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.
if result in error, we only going to warn them? instead of displaying it in the UI?
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.
leftover from previous implementation that was shuffled around. I will updated it
Hi, |
Content:
Added ability in demo website to load map/data using URLs
Refactored the way we load samples using promises
This diff will add the ability to load data and kepelr.gl files using urls. It is currently being implemented only in the demo app. once we agree upon UI and implementation we can move forward and embed the code into Kepler.gl core framework.
The implementation takes advantage of the existing load file action defined in kepler.gl.
I refactored the way we load config and data for samples using promises.
New modal dialog for load from URLS
This work resolves #256