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

[geo] Add Deckgl GeoJson layer #4068

Closed
wants to merge 16 commits into from
Closed

[geo] Add Deckgl GeoJson layer #4068

wants to merge 16 commits into from

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Dec 15, 2017

Added DeckGLs GeoJson layer as a new visualization type

@mistercrunch

@hughhhh hughhhh changed the title Add Deckgl GeoJson layer [geo] Add Deckgl GeoJson layer Dec 15, 2017
Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

We'll have to do a bit of thinking as to how we allow people to define/override the properties of the GeoJSON

@@ -514,6 +514,16 @@ export const controls = {
}),
},

geojson: {
type: 'SelectControl',
label: t('GeoJSON'),
Copy link
Member

Choose a reason for hiding this comment

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

"GeoJSON column"

],
},
{
label: t('Grid'),
Copy link
Member

Choose a reason for hiding this comment

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

This section/controls are not relevant for the GeoJSON viz

],
},
],
controlOverrides: {
Copy link
Member

Choose a reason for hiding this comment

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

This override is not relevant here

const data = payload.data.geojson.features.map(d => ({
...d,
properties: {
fillColor: [c.r, c.g, c.b, 255 * c.a],
Copy link
Member

Choose a reason for hiding this comment

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

The GeoJSON may have its own colors defined, if that's the case the user may or may not want to override it. I feel like we'll need an intricate "GeoJSON properties" control where the user could decide to override or not properties like fillColor, strokeColor, strokeWidth, ...

https://github.com/uber/deck.gl/blob/master/docs/layers/geojson-layer.md

[VIZ_TYPES.deck_screengrid]: require('./deckgl/screengrid.jsx'),
[VIZ_TYPES.deck_grid]: require('./deckgl/grid.jsx'),
[VIZ_TYPES.deck_hex]: require('./deckgl/hex.jsx'),
area: require('./nvd3_vis.js'),
Copy link
Member

Choose a reason for hiding this comment

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

Let's use VIZ_TYPES here, I think you picked the wrong side of the merge conflict.

const data = payload.data.geojson.features.map(d => ({
...d,
properties: {
fillColor: [c.r, c.g, c.b, 255 * c.a],
Copy link
Member

Choose a reason for hiding this comment

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

I still feel like there should be a way to not override the color defined in the GeoJSON (if any). We need better mapping and precedence rules here.

First, a mapping from the spec to deck.gl's supported props.

Then, clearable colorpickers (we could use opacity=0 as a hint to not override, opacity=0 could be the default), only if opacity!=0 do we override the GeoJSON provided colors.

As a last resort, we should have a color if opacity=0 and color isn't defined in the geojson.

All of this should be made clear in the controls tooltip.

This is hard to model, let's chat about it.

@hughhhh
Copy link
Member Author

hughhhh commented Dec 20, 2017

Moving -> #4097

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