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: pass extension props through #3307

Merged
merged 2 commits into from
Jul 2, 2019

Conversation

Pessimistress
Copy link
Collaborator

Follow up of #3305

Change List

  • Allow extensions to pass props from a composite layer to sub layers
  • Tests

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.5%) to 79.125% when pulling bdb9328 on x/composite-layer-extensions into f256dc0 on master.

newProps[key] = propValue;
if (propDef && propDef.type === 'accessor') {
newProps.updateTriggers[key] = this.props.updateTriggers[key];
// Unwrap sublayer objects before passing to accessor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just my 2 cents:

This adds a "hidden" unwrapObject method to some layers which feels a little iffy (Layer is already complex and contains a ton of methods some of which are ripe for cleanup).

The comment here didn't help me understand what "unwrap" means or why it is needed. Also could be valuable to point out if this just a hack for extensions or something more generically valuable for CompositeLayers? Should this be documented at some point? At least add some form of TODO?

@@ -100,6 +100,10 @@ export default class PolygonLayer extends CompositeLayer {
});
}

unwrapObject(object) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reason this one in particular looks rather hacky...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is. I'll clean up in a follow up PR.

@Pessimistress Pessimistress merged commit 5e694fa into master Jul 2, 2019
@Pessimistress Pessimistress deleted the x/composite-layer-extensions branch July 2, 2019 19:21
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.

3 participants