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

Composite layer interface and props transfer #438

Merged
merged 2 commits into from
Mar 18, 2017

Conversation

howtimeflies0
Copy link

Change the interface of GeoJson layer and Polygon layer to clear the confusion
Change the way composite layers pass their props to their underlying layers. Now it requires explicitly passing. Layers affected are the GeoJsonLayer, PolygonLayer, GridLayer, and HexagonLayer.

Docs change will be in an updated #436

@Pessimistress @ibgreen @gnavvy

…confusion;

Change the way composite layers pass their props to their underlying layers. Now it requires explicitly passing in renderLayers()
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Thanks for getting this in so quickly!

const drawPoints = pointFeatures && pointFeatures.length > 0;
const drawLines = lineFeatures && lineFeatures.length > 0;
const hasPolygonOutline = polygonOutlineFeatures && polygonOutlineFeatures.length > 0;
const hasPolygonLine = polygonOutlineFeatures && polygonOutlineFeatures.length > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

hasPolygonLines?


const polygonWireframeLayer = wireframe &&
extruded &&
hasPolygon &&
new SolidPolygonLayer(Object.assign({}, this.props, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to get rid of the Object.assign here.

polygonOutlineLayer,
lineLayer,
polygonLineLayer,
pathLayer,
pointLayer
].filter(Boolean);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should to add the filtering by default inside deck.gl - Because I see so many layers doing .filter(Boolean) thing now. React supports null/empty elements, we should too. I think it is a one line change, could even go into this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Agree. but let's do it in a different PR

@@ -35,6 +35,7 @@ const defaultProps = {
elevationRange: defaultElevationRange,
elevationScale: defaultElevationScale,
getPosition: x => x.position,
extruded: false,
fp64: false
// AUDIT - getWeight ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove?

@howtimeflies0 howtimeflies0 merged commit dcd1434 into master Mar 18, 2017
@howtimeflies0 howtimeflies0 mentioned this pull request Mar 20, 2017
@howtimeflies0 howtimeflies0 deleted the composite-interface branch March 22, 2017 15:55
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