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

[SIP-5] Remove references to slice from all deck.gl components. #6039

Merged
merged 19 commits into from
Oct 12, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Oct 5, 2018

Decouple all visualization components from the parent container (Currently passed in asslice). This is the last part of SIP-5 #5680 and is important to unblock the work on chart plugin system.

This PR is slightly different from other SIP-5 PRs because those also decouple visualization code from formData, but for this PR, I am leaving formData as-is.

Also, create utility function createAdaptor particularly for deck.gl components. This function handles extracting the necessary fields from slice, then calls ReactDOM and render the given React Component onto the target DOM element, passing it the transformed props.

Most of deck.gl components have lots of similarity and most can be defined by 3 factors:

  • getLayer function
  • getPoints function
  • whether it uses DeckGLContainer or CategoricalDeckGLContainer
    so I create two factory functions in factory.jsx that create a React component given getLayer and getPoints. See arc.jsx for example of how the code was simplified.

The two exceptions are ScreenGrid and GeoJson that are different.

I have verified with example data and they are working.

@betodealmeida @mistercrunch @xtinec @williaster @conglei

@codecov-io
Copy link

codecov-io commented Oct 5, 2018

Codecov Report

Merging #6039 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6039   +/-   ##
=======================================
  Coverage   77.59%   77.59%           
=======================================
  Files          47       47           
  Lines        9496     9496           
=======================================
  Hits         7368     7368           
  Misses       2128     2128

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b4cf85...bc036d6. Read the comment docs.

@kristw
Copy link
Contributor Author

kristw commented Oct 8, 2018

Could somebody take a look at this one, please?

document.getElementById(slice.containerId),
);
}

module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

curious if this works with export default { ... } syntax (I think maybe would need to update the vis index/promise file, nbd if you don't want to do that in this PR)

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

This LGTM with some comments that might be out of scope.

@mistercrunch @betodealmeida do you want to take a look since you're more familiar with this code?

@kristw kristw closed this Oct 10, 2018
@kristw kristw reopened this Oct 10, 2018
@kristw
Copy link
Contributor Author

kristw commented Oct 11, 2018

I am hoping to merge this one to unblock the embeddable chart works. @betodealmeida @mistercrunch if you have any comment or concern, could you let me know by EOD, please? Otherwise I will assume it is good to proceed.

@williaster
Copy link
Contributor

gonna merge since we haven't heard anything.

@williaster williaster merged commit 2a7b64f into apache:master Oct 12, 2018
@kristw kristw deleted the kristw-deck branch November 1, 2018 21:25
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
…he#6039)

* start removing slice.

* migrate arc

* refactor arc

* refactor path

* refactor deck polygon

* remove commented code

* refactor factory function

* refactor grid

* refactor hex

* refactor polygon

* refactor scatter

* refactor geojson

* refactor screengrid

* remove unnecessary nesting

* add proptypes

* add proptypes

* refactor deck.gl Multi

* fix lint

* Use export syntax instead of module.exports
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants