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

Permalink fixes #38

Merged

Conversation

neokore
Copy link

@neokore neokore commented Nov 21, 2019

No description provided.

filename: file.name,
status: 'uploading',
metadata: null,
provider: cloudProvider.name

Choose a reason for hiding this comment

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

👍 it looks like a nice approach, passing around the provider

@@ -380,12 +385,25 @@ export function exportFileToCloud(providerName) {
.then(
response => {
if (cloudProvider.shareFile) {
dispatch(push(`/${response.url}`));
const responseUrl = (cloudProvider.getMapPermalink)
? cloudProvider.getMapPermalink(response.url, false)
Copy link

@VictorVelarde VictorVelarde Nov 21, 2019

Choose a reason for hiding this comment

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

If we're gonna start sharing / overriding behavior, maybe we should think about a base 'provider abstract class' + inheritance, so externally this would be just cloudProvider.getMapPermalink.

That would be also a way to officially declare a common provider interface.

But let's wait a bit for that...

Copy link
Author

Choose a reason for hiding this comment

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

Totally agree

</div>
)}
{metaUrl && [
(<SharingUrl key={0} url={sharingLink} message={'Share your map with other users'}/>),
(<a href={folderLink} target="_blank" rel="noopener noreferrer" style={{textDecoration: 'underline'}}>Go to your Kepler.gl Dropbox folder</a>)
(folderLink && <a href={folderLink} target="_blank" rel="noopener noreferrer" style={{textDecoration: 'underline'}}>Go to your Kepler.gl {providerName} page</a>)

Choose a reason for hiding this comment

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

Nice, providerName fits well here!

Choose a reason for hiding this comment

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

With this 'old approach' (not our first one), the renderMeta method in carto-provider won't be used (delete or at least comment it out). But we might want to discuss about that in the future, as I think it is bit cleaner than this current approach.

@neokore neokore merged commit 1c9f6d6 into feature/share-to-carto-refactored Nov 22, 2019
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.

2 participants