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

Update the overlay API so that required props can actually be required props #28

Closed
vicapow opened this issue Nov 25, 2015 · 2 comments
Closed

Comments

@vicapow
Copy link
Contributor

vicapow commented Nov 25, 2015

Currently, the <MapGL> component clones it's children and adds the necessary props. This causes all props to be required as optional since they first get created once without the necessary props. To solve this, we can either:

1.) Use React's context feature.

<MapGL ...mapProps>
    <Overlay locations={locations}/>
</MapGL>

This would require changes to the implementation of existing overlays. It also means we wont be able to use PureRenderMixin since it doesn't know about context. It's also seems to be discouraged to use context in React in general.

2.) Pass the overlays to a callback properly of the <MapGL> component.

<MapGL ...mapProps overlays={(viewport) => {
  <Overlay ...viewport locations={locations} />
}/>

This would cause changes to the usage of overlays but requires no changes to the overlays themselves. It also has the advantage of supporting PureRenderMixin.

@vicapow
Copy link
Contributor Author

vicapow commented Nov 26, 2015

see: f7741b0

@vicapow
Copy link
Contributor Author

vicapow commented Dec 2, 2015

Ended up changing this up a bit from the original proposal. see: https://gist.github.com/vicapow/00017553e92f613d5361 for details

This has landed with: fe329bc

@vicapow vicapow closed this as completed Dec 2, 2015
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

No branches or pull requests

1 participant