[chore] Refactored cloud provider flow for performance and multi provider support #2436
Conversation
…ider support Signed-off-by: Giuseppe Macri <gmacri@foursquare.com>
macrigiuseppe
changed the title
| @@ -78,7 +84,6 @@ export default class Provider { | |||
| displayName: string; | |||
| icon: ComponentType<IconProps>; | |||
| thumbnail: Thumbnail; | |||
| getManagementUrl?: () => string; | |||
There was a problem hiding this comment.
converted to an actual method
src/components/src/kepler-gl.tsx
Outdated
| availableProviders = createSelector( | ||
| (props: KeplerGLProps) => props.cloudProviders, | ||
| providers => | ||
| Array.isArray(providers) && providers.length | ||
| ? { | ||
| hasStorage: providers.some(p => p.hasPrivateStorage()), | ||
| hasShare: providers.some(p => p.hasSharingUrl()) | ||
| } | ||
| : {} | ||
| ); |
There was a problem hiding this comment.
we no longer need to inject providers since components can use the custom hook to access cloud list provider
| {...geoCoderPanelFields} | ||
| index={index} | ||
| unsyncedViewports={true} | ||
| <CloudListProvider providers={cloudProviders}> |
There was a problem hiding this comment.
just wrapping the content using the CloudListProvider
| @@ -355,9 +346,6 @@ export default function ModalContainerFactory( | |||
| onClose={this._closeModal} | |||
| onFileUpload={this._onFileUpload} | |||
| onLoadCloudMap={this._onLoadCloudMap} | |||
| cloudProviders={this.providerWithStorage(this.props)} | |||
There was a problem hiding this comment.
not needed anymore
| } | ||
| `; | ||
|
|
||
| export const CloudHeader = ({provider, onBack}) => { |
There was a problem hiding this comment.
extracted this content from load-storage-map
| } | ||
| `; | ||
|
|
||
| export const CloudItem = ({vis, onClick}) => { |
There was a problem hiding this comment.
extract from load-storage-map
| } | ||
| `; | ||
|
|
||
| export const CloudMaps = ({provider, onSelectMap, isLoading, maps, error}) => { |
There was a problem hiding this comment.
extract from load-storage-map
| @@ -78,7 +79,7 @@ const StyledUserName = styled.div` | |||
| `; | |||
|
|
|||
| interface OnClickProps { | |||
| onClick?: React.MouseEventHandler<HTMLDivElement>; | |||
| onClick?: React.MouseEventHandler<HTMLButtonElement>; | |||
There was a problem hiding this comment.
since we are passing props to a button makes more sense to target HTMLButtonElement
| const notification = useMemo( | ||
| () => ({ | ||
| type: 'error', | ||
| message: error, | ||
| id: 'cloud-export-error' | ||
| }), | ||
| [error] | ||
| ); |
There was a problem hiding this comment.
performance improvement
|
|
||
| /** | ||
| * Set current cloudProvider | ||
| */ |
There was a problem hiding this comment.
we no longer need these actions since we are handling everything through hooks and provider
macrigiuseppe
force-pushed
the
refactor_cloud_provider
branch
from
b0566e8
to
661b928
Compare
Signed-off-by: Giuseppe Macri <gmacri@foursquare.com>
macrigiuseppe
force-pushed
the
refactor_cloud_provider
branch
from
661b928
to
ca475f8
Compare
| @@ -0,0 +1,59 @@ | |||
| // @ts-nocheck | |||
There was a problem hiding this comment.
Are we fine with placing .spec.tsx not in test/ folders?
There was a problem hiding this comment.
working on it. i want to make the case about colocating tests next to their source because it improves developer ergonomic and make writing tests much quicker and simpler.
I had no problem mocking custom hooks in this setup using jest but if i move the test in the Test folder i need to mock kepler.gl/components with several side effects
There was a problem hiding this comment.
we are good to go
| if (provider.getAccessToken()) { | ||
| setError(null); | ||
| setIsLoading(true); | ||
| setError(null); |
There was a problem hiding this comment.
is this extra call to setError(null); required?
There was a problem hiding this comment.
this is used to clear the error message when a user switch from a provider that triggered an error, e.g. timeout, to a different provider.
We hide the previous error and show the loading icon
There was a problem hiding this comment.
I mean the same setError(null); is called in the line 120 & 122
There was a problem hiding this comment.
oh i didn't realize that. thank you for catching it
|
|
||
| const onLogin = useCallback(async () => { | ||
| try { | ||
| const user = await provider.login(); |
There was a problem hiding this comment.
When I try to log into Dropbox but click Cancel, the popup doesn't close and redirects to the new kepler.gl view.
There was a problem hiding this comment.
let me look into this!
| isReady={isReady} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </StyledExportSection> | ||
| {isProviderLoading || providerError ? ( | ||
| <StatusPanel | ||
| isLoading={isProviderLoading} | ||
| error={providerError} |
There was a problem hiding this comment.
It looks like an error error generated by one provider is displayed and not cleared when a user switches to the second provider.
There was a problem hiding this comment.
will look into this as well
Signed-off-by: Giuseppe Macri <gmacri@foursquare.com>
Signed-off-by: Giuseppe Macri <gmacri@foursquare.com>
In this PR i refactored the business logic to: