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 ZoomToMaxExtentButton component #190

Merged
merged 9 commits into from
Oct 5, 2015

Conversation

chrismayer
Copy link
Contributor

As described and discussed in #102 this PR adds a ZoomToMaxExtentButton component.

@simboss simboss added the ready label Oct 1, 2015
@mbarto
Copy link
Contributor

mbarto commented Oct 1, 2015

Please, check Travis build: it's failing with this error:

web/client/examples/viewer/plugins/index.jsx
90:12 error Line X: Adjacent JSX elements must be wrapped in an enclosing tag

@mbarto
Copy link
Contributor

mbarto commented Oct 1, 2015

From an initial look at the code, it cannot work. Did you test that it is working in either leaflet or openlayers viewer?

@chrismayer
Copy link
Contributor Author

Did you test that it is working in either leaflet or openlayers viewer?

Sure I did. All looked fine to me. What is wrong with the code?

@mbarto
Copy link
Contributor

mbarto commented Oct 1, 2015

I will checkout the code and do a direct test. I will let you know.


// In addition to the state, 'connect' puts 'dispatch' in our props.
// connect Redux store slice with map configuration
module.exports = connect((state) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be preferrable to bind properties in the plugins index.js as we do for other plugins, instead of connecting directly to the store

@mbarto
Copy link
Contributor

mbarto commented Oct 1, 2015

I think it works only when maxExtent is not defined.
When there is a maxExtent you invoke the action with maxExtent and a -1 zoom, but if you look at both OpenLayers and Leaflet Map.jsx components, you will see that:

  • bbox is not used to change current map zoom / center
  • only center and zoom are used for that purpose and -1 is not a valid zoom value

This implements a class skeleton for a button, which should be used to
zoom a map to its max. extent. This is only a skeleton to integrate the
UI (button) without any functionality.
This adds the ZoomToMaxExtentButton to the examples viewer instances.
In case of clicking the ZoomToMaxExtent button the map zooms to the max.
extent which is defined in the map's config object. If no max. extent is
defined the map will be zoomed to zoom level '1'.
  - Bind properties in the plugins index.js as we do for other plugins
  - Call action as a property without directly calling dispatch
@chrismayer
Copy link
Contributor Author

Okay I'll bind the actions the other way you described.

What would be your favourite strategy to zoom the map by center, since

  • bbox is not used to change current map zoom / center
  • only center and zoom are used for that purpose and -1 is not a valid zoom value

Would extending _updateMapPositionFromNewProps in the Map.jsx files (OpenLayers and Leaflet) be an option so they check if only the extent object bbox has been changed and zoom to it?

@mbarto
Copy link
Contributor

mbarto commented Oct 2, 2015

Hi, Christian.
The most important thing is that the application state MUST be consistent. So:

  • in the reducer you need to set all the properties (zoom, center, bbox) to represent the same map; since you have a bbox, you need to calculate the related center and zoom, and set all the 3 properties on the new state
  • then you don't need to change _updateMapPositionFromNewProps

Consider that my desire is to remove the bbox from the state and make it a calculated property of center and zoom, so those 2 are the important properties for map navigation.

This ensures max. extent is set by corresponding center and zoom values
which are calculated by the 'maxExtent' property of the map's config.
@chrismayer
Copy link
Contributor Author

Hi @mbarto,
I tried to address your desire. So now the max. extent will be set by center and zoom, which are calculated from maxExtent.

@mbarto
Copy link
Contributor

mbarto commented Oct 2, 2015

Looks good. I will try to test it on monday, and then merge it. Thank you.

@mbarto mbarto merged commit 9c3e81e into geosolutions-it:master Oct 5, 2015
@mbarto mbarto removed the ready label Oct 5, 2015
@mbarto
Copy link
Contributor

mbarto commented Oct 5, 2015

Hi Christian, I did a couple of minor adjustements and merged your work. Thank you.

ale-cristofori pushed a commit to ale-cristofori/MapStore2 that referenced this pull request Apr 3, 2024
…-2023" (geosolutions-it#190)

* Update revision to latest master 7-12-2023

* geosolutions-it#189: update version in version.txt and package.json

* geosolutions-it#189: enable details panel in dashboard by editing the localConfig.json file

* geosolutions-it#189: resolve review comments:
Description:
- Remove all font-awesome style sheet in html files
- Update localConfig based on migration guide to 2024.01.00

* geosolutions-it#189: add npmrc file to project

* Update version.txt

---------

Co-authored-by: Tobia Di Pisa <tobia.dipisa@geosolutionsgroup.com>
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