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

Add a BoundsMap (<Map> alternative) component #72

Merged
merged 6 commits into from
Sep 28, 2015

Conversation

uniphil
Copy link
Contributor

@uniphil uniphil commented Sep 22, 2015

This pull request adds a component which sets the map view via a dynamic bounds prop instead of by center and zoom.

An alternative implementation could add the bounds prop to the existing Map component, and let users to either set bounds or center+zoom to control the map view. I started writing it that way, but found it required too many if/else blocks and helper functions to do the right thing for my taste. If you would prefer it that way, I can change it back.

At work we use map.fitBounds almost exclusively for setting the view, so something like this component is essential for our use of react-leaflet.

The `Map` component only supports setting and updating the map view by
changing the map `center` and `zoom` props. It is often useful to set
the map view by specifying some bounds that must be contained instead.

For example: loading a full-screen map to show a country.

This commit adds a `BoundsMap` component which extends `Map` and simply
takes a `bounds` prop (of type `src/lib/types/bounds`) instead of
`center` and `zoom` props.

Tests for this component were adapted from those for `Map`.
Shows that the `bounds` property is dynamic
}
}

componentDidUpdate({ bounds: oldBounds }) {
Copy link

Choose a reason for hiding this comment

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

I think this would be better as componentDidUpdate(prevProps), as it makes things more clear to anybody that wants to extend BoundsMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@PaulLeCam
Copy link
Owner

Hi, thanks for the PR.

Is there any particular reason for doing it as a separate component rather than adding this behavior to Map?
I suppose the fitBounds() call could be made after the center and zoom are set, would that suit your use case?

@uniphil
Copy link
Contributor Author

uniphil commented Sep 24, 2015

Calling fitBounds manually works, but it moves all the bookkeeping logic into our app, which we are trying to avoid with React. The use-case here: We have "international", "country", and "project" views for geocoded data in Asia. We use react-router to draw chrome around the map and features on the map depending on the view. It's nice to just update bounds like the rest of our data and have react/react-leaflet deal with it.

If you would prefer, I can modify Map to handle a bounds prop instead of adding this BoundsMap component. In that case:

  • Setting bounds AND (center OR zoom) props is probably a mistake by the user
    • it would be nice to log a warning. I don't see any warnings currently used, and console statements make the linter complain. Do you have a preferred way to do this? Can I add a warn.js module with /* eslint no-console: 0 */ that exports a warn function?
    • If this case happens, which props should react-leaflet use to set the view -- bounds, or (center and zoom)?

@PaulLeCam
Copy link
Owner

I think it makes sense to have the Map component handle dynamic bounds and maxBounds props, it could then be used either using bounds or the center/zoom combination.

I'm against adding any specific logic in the components regarding specific use cases as in my option the components provided in this lib should always be "dumb" ones, the "smart" ones being in the app.
Regarding the case of setting bounds, center and zoom, I think the Map component would just apply all of them, it's up to the developer to use the APIs they need.

If you're up for it, would you mind updating your PR to add this to Map? Otherwise I'll try to experiment around it when I have some time.

As requested, there is no validation of bounds vs. center/zoom modes to set the view. Last updated wins.
@uniphil
Copy link
Contributor Author

uniphil commented Sep 28, 2015

Moved it all to dynamic bounds on <Map>: no validating or branching logic based on props -- last update wins.

PaulLeCam added a commit that referenced this pull request Sep 28, 2015
Add a BoundsMap (<Map> alternative) component
@PaulLeCam PaulLeCam merged commit 9729cdc into PaulLeCam:master Sep 28, 2015
@uniphil uniphil deleted the map-component-bounds branch September 29, 2015 01:06
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