GoogleDriveProvider - #832
Conversation
913ae5a to
76682cf
Compare
0008766 to
fec8cb8
Compare
fec8cb8 to
d579d26
Compare
Signed-off-by: Ib Green <ibgreen@Ibs-MacBook-Pro.local>
d579d26 to
9acbe43
Compare
|
@macrigiuseppe This PR is now ready for review. It would be good to get some initial feedback on the way additional tokens are being managed, with new environment variables etc. |
|
@macrigiuseppe Added screenshot |
ibgreen
changed the title
| * To access Google Drive, specify it in scopes | ||
| * param scopes = ['https://www.googleapis.com/auth/drive.file']; | ||
| */ | ||
| constructor(appName, clientId, appKey, scopes = SCOPES) { |
There was a problem hiding this comment.
We should try to make to make this consistent across all cloud providers.
Right now Dropbox provider accepts an icon where here we have different parameters.
I think we can follow a more flexible pattern:
constructor(appName, clientId, options = {})
options can handle all different requirement for each cloud provider
| * param scopes = ['https://www.googleapis.com/auth/drive.file']; | ||
| */ | ||
| constructor(appName, clientId, appKey, scopes = SCOPES) { | ||
| super({ |
There was a problem hiding this comment.
For default parameters I would create a constant and merge it with constructor’s postmasters
| * @returns {Promise<void>} | ||
| */ | ||
| async uploadFile({blob, name, isPublic = true}) { | ||
| const mimeType = 'application/json'; |
There was a problem hiding this comment.
This can be defined outside the scope of this method.
Actually it could be defined in a cloud provider constant file since other providers may take advantage of it
| const metadata = { | ||
| name, // Filename on Google Drive | ||
| mimeType, // mimeType on Google Drive | ||
| parents: ['root'] // Folder ID at Google Drive |
| }; | ||
|
|
||
| // The body of the post request will be this form | ||
| const form = new FormData(); |
There was a problem hiding this comment.
I would encapsulate the logic to generate the body into an helper method
| // To access Google Drive, specify it in scopes | ||
| // @param scopes = ['https://www.googleapis.com/auth/drive']; | ||
| constructor({clientId, appKey, name, appName, icon, discoveryDocs, scopes = []}) { | ||
| // All cloud-providers providers must implement the following properties |
There was a problem hiding this comment.
I’m not clear what the hierarchy is here. My understanding is the following:
GoogleDroveProvider and DropboxProvider are peers and GoogleProvider and DropboxSdk are their respective parents.
But constructors don’t reflect such hierarchy, why google provider have an icon given it is an api layer?
If my first assumptions is wrong, can you clarify in the PR description the role of GoolgeProvider and GoogleCloudProvider?
I only see google provider in it
| this._getUserData(googleUser); | ||
| this._onLoginStatusChange(); | ||
| onCloudLoginSuccess(this.name); | ||
| }).catch (error => { |
There was a problem hiding this comment.
Can you call onCloudLoginFailure?
|
Hi @macrigiuseppe and @ibgreen. Regarding to the UI of the 'tiles', we have this WIP here: https://github.com/keplergl/kepler.gl/blob/660bc42a53383f276f1f8b4c2b2ec1200db52fae/examples/demo-app/src/components/sharing/cloud-tile.js that might be useful. Apart from the space between the tiles, we also added the 'username', with a new |
3 participants
(Note: This is currently a stacked diff on top of the
cloud-provider-refactorbranch #831. Should be rebased to master before landing.)To test/integrate a Google client id and app keyneeds to be registered for kepler.gl. Optionally, I can provide the client id for the integration I have set up (which could make some sense as I may keep working on the Google integration for a while).
SOME HIGHLIGHTS:
GoogleProviderdynamically loads thegapi(Google API) library with a newloadScriptutil, partly to help avoid bundle size impact from linking in multiple cloud provider SDKs.GoogleProviderimplements OAUTH2 login viagapi.REMAINING:
GoogleProvidershould correctly implement file upload (currently lists google drive files instead of uploads them).