Enable zoom by box selection #319
Conversation
components/MapSelection.jsx
Outdated
| condition: ol.events.condition.shiftKeyOnly | ||
| condition: () => { | ||
| return ["ZoomIn", "ZoomOut"].includes(this.props.currentTask) || ol.events.condition.shiftKeyOnly; | ||
| } |
There was a problem hiding this comment.
I think you can generally simplify the condition to return true, shiftKeyOnly looks like leftover code, and I don't think it makes any sense here
There was a problem hiding this comment.
I thought it was used by the shift+drag zoom already implemented but nope. I have removed all the condition
plugins/ZoomButtons.jsx
Outdated
| if (this.props.currentTask !== null && this.props.currentTask === this.state.task && this.state.disabled) { | ||
| this.props.setCurrentTask(null); | ||
| } | ||
| if (this.props.currentTask === this.state.task && prevProps !== this.props) { |
There was a problem hiding this comment.
If you write this.props.click && this.props.click !== prevProps.click here and directly use this.props.click.coordinate below you can simplify the code
|
A general remark: Does this conflict/overlap with the already implemented shift+drag zoomin? |
No, it does not conflict with this feature. I thought it was implemented with the |
plugins/ZoomButtons.jsx
Outdated
| if (this.props.currentTask !== null && this.props.currentTask === this.state.task && this.state.disabled) { | ||
| this.props.setCurrentTask(null); | ||
| } | ||
| if (this.props.currentTask === this.state.task && prevProps !== this.props && this.props.click !== prevProps.click) { |
There was a problem hiding this comment.
this.props.currentTask === this.state.task && this.props.click && this.props.click !== prevProps.click
|
I'd propose these additional changes [1], what do you think? [1] db228da |
Yes I agree with all of your changes, thanks. For |
It will just re-center the map without changing the zoom level, which I think is not a bad behaviour. |
|
Manually merged, thanks! |
Hi,
This PR introduces a new
enableZoomByBoxSelectionoption forZoom(In|Out)Pluginto allow user to zoom by box selection. I needed to updateMapSelectionto make it work, I don't think I breaks something for other plugins that use this component but I'm not sure...I tried to reproduce behavior for Zoom buttons in QGIS Desktop.
Here is a screencast with
enableZoomByBoxSelection: falseforZoomInPlugin(actual behavior) andenableZoomByBoxSelection: trueforZoomOutPlugin:Thanks for the review !
This feature has been funded by Grand Lyon Métropole https://www.grandlyon.com/