-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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] Introduce Foursquare cloud provider #2437
Conversation
1d9034e
to
5608c0f
Compare
661b928
to
ca475f8
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.
Nicely done! Minor nits, feel free to ignore.
- `props.name` **[string][28]** | ||
- `props.displayName` **[string][28]** | ||
- `props` **[object][27]** | ||
- `props.name` **[string][28]** |
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.
What are those numbers in brackets? Look like link tags - in that case shouldn't those tags be defined below? Or maybe this is a different markdown dialect?
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.
the tags are defined at the end of the file
[27]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object |
const style = { | ||
background: 'black' | ||
}; | ||
export default class UnfoldedIcon extends Component { |
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.
FoursquareIcon? (Nit: If we stop using default exports we won't be making such mistakes)
@@ -0,0 +1,41 @@ | |||
import React, {Component} from 'react'; |
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.
Should we just call this foursquare-icon.js
const STUDIO_AUTH_AUDIENCE = 'https://foursquare.com/api/'; | ||
const STUDIO_AUTH_SCOPE = 'openid profile email'; | ||
|
||
const STUDIO_KEPLER_GL_IMPORT_SOURCE = 'kepler.gl-raw'; |
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.
What does kepler.gl-raw mean? Is it a URL path, a parameter value, etc? Maybe add comment before this constant,
}; | ||
} | ||
|
||
export default class FsqStudioProvider extends Provider { |
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 vote for FoursquareProvider
FSQ_STUDIO_DOMAIN, | ||
FSQ_STUDIO_API_URL, | ||
FSQ_STUDIO_USER_MAPS_URL | ||
} = AUTH_TOKENS; |
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 no longer just AUTH_TOKENS. To make the code easier to follow, maybe consider renaming to CLOUD_PROVIDERS_CONFIGURATION
or similar?
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.
yeah we can do that
examples/demo-app/webpack.config.js
Outdated
@@ -69,7 +69,11 @@ const CONFIG = { | |||
'MapboxAccessToken', | |||
'DropboxClientId', | |||
'MapboxExportToken', | |||
'CartoClientId' | |||
'CartoClientId', | |||
'FSQStudioClientId', |
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 recommend just going with Foursquare
.
'FSQStudioClientId', | |
'FoursquareClientId', |
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.
yeah
@@ -133,10 +135,10 @@ export default class Provider { | |||
/** | |||
* This method is called to determine whether user already logged in to this provider | |||
* @public | |||
* @returns true if a user already logged in | |||
* @returns {Promise<string>} return the access token if a user already logged in |
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 we add types in typescript we should not add them to the TSDoc.
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 being used to generate docs: https://docs.kepler.gl/docs/api-reference/cloud-providers/cloud-provider#getaccesstoken
@@ -253,8 +255,7 @@ export default class Provider { | |||
* } | |||
*/ | |||
async downloadMap(loadParams): Promise<{map: SavedMap; format: string}> { | |||
// @ts-expect-error | |||
return; | |||
return Promise.reject('You must implement downloadMap'); |
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.
Nit: In my opinion it is cleaner to just throw an error and the async function will convert that to a rejected promise.
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.
yeah i can do that
@@ -1,5 +1,25 @@ | |||
// Copyright (c) 2023 Uber Technologies, Inc. | |||
// | |||
// Permission is hereby granted, free of charge, to any person obtaining a copy |
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.
Do we needs to add this?
- If we do, can't we just add a one liner
// Copyright (c) 2023 Uber Technologies, Inc., MIT license
? That is what we do in other vis.gl frameworks. - Should this be a FSQ copyright? Or is the year wrong? I assume Uber didn't do anything with this code in 2023.
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 guess this is an automatic license added by uber-licence
. Should it be disabled?
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 part of the open source code. only files in the example folder should have the FSQ copyright
@@ -1,5 +1,25 @@ | |||
// Copyright (c) 2023 Uber Technologies, Inc. | |||
// | |||
// Permission is hereby granted, free of charge, to any person obtaining a copy |
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 guess this is an automatic license added by uber-licence
. Should it be disabled?
ac5eda6
to
868c90e
Compare
2b3ca6b
to
7cdfe35
Compare
Signed-off-by: Giuseppe Macri <[email protected]>
7cdfe35
to
9bc24cd
Compare
Signed-off-by: Giuseppe Macri <[email protected]>
This PR introduces a new FSQ Studio provider to be able to store and fetch maps from Foursquare Studio