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.

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

[Feat] Load cloud map with provider #947

Merged
merged 17 commits into from
Feb 28, 2020
Merged

[Feat] Load cloud map with provider #947

merged 17 commits into from
Feb 28, 2020

Conversation

heshan0131
Copy link
Contributor

@heshan0131 heshan0131 commented Feb 11, 2020

This implements #943

add dropbox user name, logout, save image, list files
implement load-storage-map modal and handling loadMaps in provider updater
Signed-off-by: Shan He [email protected]

@heshan0131
Copy link
Contributor Author

heshan0131 commented Feb 11, 2020

  • kepler.gl calls provider.listMaps to get a list of visualizations shown in the load storage map gallery

  • when user select a visualization, kepler.gl calls provider.downloadMap and pass in theloadParams property from each visualization object

  • provider.downloadMap should return

    const response = {
      map: {
         datasets: [], 
         config: {},
         info: {
            title: 'test map',
            description: 'Hello this is my test dropbox map'
          }
      },
      // available option: 'csv', 'geojson', 'row', 'keplergl'
      format: 'keplergl'
    };

Each dataset should either

  1. {data: {...}, info: {}} where data can be csv string, geojson object, or row object. You need to pass in the correspondant format to tell kepler how to process it

  2. dataset object saved directly from KeplerGlSchema.save

@heshan0131 heshan0131 force-pushed the provider-load-map branch 2 times, most recently from 37fb0a8 to 7c8c1f6 Compare February 17, 2020 04:30
@ibgreen ibgreen changed the base branch from backend-storage-ui to master February 17, 2020 20:57
@ibgreen
Copy link
Collaborator

ibgreen commented Feb 24, 2020

I have a concern with how errors (exceptions) are handled in this PR.

Exceptions should be clean instances of Error constructed with a string parameter, not some arbitrary object.

I see a lot of downsides to this custom setup and no upsides. (If necessary the error instance can be annotated with additional fields like status.)

@macrigiuseppe
Copy link
Collaborator

  • kepler.gl calls provider.listMaps to get a list of visualizations shown in the load storage map gallery
  • when user select a visualization, kepler.gl calls provider.downloadMap and pass in theloadParams property from each visualization object
  • provider.downloadMap should return
    const response = {
      map: {
         datasets: [], 
         config: {},
         info: {
            title: 'test map',
            description: 'Hello this is my test dropbox map'
          }
      },
      // available option: 'csv', 'geojson', 'row', 'keplergl'
      format: 'keplergl'
    };

Each dataset should either

  1. {data: {...}, info: {}} where data can be csv string, geojson object, or row object. You need to pass in the correspondant format to tell kepler how to process it
  2. dataset object saved directly from KeplerGlSchema.save

can we add the above info to DEVELOPER.MD?

@macrigiuseppe
Copy link
Collaborator

I have a concern with how errors (exceptions) are handled in this PR.

Exceptions should be clean instances of Error constructed with a string parameter, not some arbitrary object.

I see a lot of downsides to this custom setup and no upsides. (If necessary the error instance can be annotated with additional fields like status.)

Hey @ibgreen, which part of the PR are you referring to?
Is there a particular piece of the code that you think we should look into?

Signed-off-by: Shan He <[email protected]>
const {params: {id, provider} = {}, location: {query = {}} = {}} = this.props;

const cloudProvider = CLOUD_PROVIDERS.find(c => c.name === provider);
if (provider) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (cloudProvider)

if (!token) {
this.login(() => this.downloadMap(loadParams));
}
const result = await this._dropbox.filesDownload(loadParams);
Copy link
Collaborator

Choose a reason for hiding this comment

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

try/catch in case of download errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to try catch here Errors will be handled by error action passed to react-palm bimap

if (thb['.tag'] === 'success' && thb.thumbnail) {
const matchViz = visualizations[pngs[thb.metadata.id] && pngs[thb.metadata.id].name];
if (matchViz) {
matchViz.thumbnail = `data:image/gif;base64,${thb.thumbnail}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's create a constant for this prefix: data:image/gif;base64,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

try {
response = await this._dropbox.usersGetCurrentAccount();
} catch (error) {
Console.warn(error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we return here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const toSave = exportMap(this.props);
console.log(toSave);
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

(f, c) => ({
// using concat here because the current datasets could be an array or a single item
datasets: f.datasets.concat(c.datasets),
// const loadFileTasks = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@macrigiuseppe
Copy link
Collaborator

@heshan0131
Notes:

  1. First time, open load from storage
    When there are no items in the cloud storage, the window is not providing a clear message to users and the UI seems half baked

Screen Shot 2020-02-27 at 10 05 08 AM

  1. When saving a map:
    a. The map preview is zoomed out and it doesn't provide a correct preview of the map.

Screen Shot 2020-02-27 at 10 12 52 AM

Screen Shot 2020-02-27 at 10 09 26 AM

b. When filling out name and description tabbing is not working, I can't jump from one field to the other and I have to use the mouse; probably tabbing doesn't work because those two inputs are not part of a form element.

  1. Loading from storage
    The cloud provider tile has a lot of white space

Screen Shot 2020-02-27 at 10 15 54 AM

  1. Loading using permalink
    It looks like we lost the loading state when using permalinks (both example map or cloud storage one).

This is from our current website:
loading-state

This is with the new changes

storage-loading-permalink

loading-state-missing

- add dropbox user name, logout, save image
- add load map from dropbox
- implement load-storage-map modal and handling loadMaps in provider updater

- implement save as

Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
Signed-off-by: Shan He <[email protected]>
@heshan0131 heshan0131 merged commit c75812d into master Feb 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the provider-load-map branch February 28, 2020 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants