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

Kuviz creation integration #717

Merged
merged 48 commits into from
Jun 6, 2019
Merged

Kuviz creation integration #717

merged 48 commits into from
Jun 6, 2019

Conversation

simon-contreras-deel
Copy link
Contributor

@simon-contreras-deel simon-contreras-deel commented May 31, 2019

Solves a part of #674

The objective is to integrate the new Kuviz class from carto-python to use it in publish method of "sharing visualizations" project. In this PR we are only integrating the create method, the rest will be integrated into other PR.

The "Kuviz publish" PR will be merged into this one #718 finishing the publication process

test/viz/test_kuviz.py Outdated Show resolved Hide resolved
test/viz/test_kuviz.py Outdated Show resolved Hide resolved
@simon-contreras-deel simon-contreras-deel changed the title Kuviz integration [WIP] Kuviz integration May 31, 2019
@simon-contreras-deel simon-contreras-deel changed the title [WIP] Kuviz integration [WIP] Kuviz creation integration May 31, 2019
@simon-contreras-deel simon-contreras-deel changed the title [WIP] Kuviz creation integration Kuviz creation integration Jun 4, 2019
simon-contreras-deel and others added 7 commits June 5, 2019 14:29
Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>
Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>
Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>
Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>
Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>
Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>
@andy-esch
Copy link
Contributor

👀

Copy link
Contributor

@andy-esch andy-esch left a comment

Choose a reason for hiding this comment

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

Looks great, just left a few minor comments.

On a high level, this class will largely be hidden from the user, correct? Will it be inside Map().publish()? How do you envision user-facing API for creating, updating, etc.?

Part of the reason for asking this is that I think we should potentially change error messages from Kuviz -> cartoframes visualizations or something like that so we're not introducing a term that's more of an implementation detail.

cartoframes/viz/kuviz.py Outdated Show resolved Hide resolved
cartoframes/viz/kuviz.py Show resolved Hide resolved
@simon-contreras-deel
Copy link
Contributor Author

About how Kuviz is used, the following PR resolves the publication: #718

list kuvizs:

kuvizs = Kuviz.all()

update / delete methods:

# from publish
kuviz** = map.publish(...)
kuviz.update(...)

# from list
kuvizs = Kuviz.all()
kuvizs[0].delete(...)

Kuviz and Visualization are not exactly the same. Kuviz are a subset of visualizations, but with different flows and final results

@simon-contreras-deel simon-contreras-deel merged commit 5f59344 into feature-sharing-viz Jun 6, 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.

4 participants