#12650: Zoom to filtered layer from the filter widget - #12707
Conversation
allyoucanmap
left a comment
allyoucanmap
left a comment
There was a problem hiding this comment.
No cleanup path handles APPLY_ZOOM_TO, so a plugged zoom-to interaction can outlive the applyFilter sibling it depends on, how to reproduce:
- open a map and add a wms layer
- add a filter widget
- connect the filter widget to the layer
- connect the zoom to action to the layer
- save the widget
- verify the zoom to is working
- edit the widget again
- disconnect only the filter interaction
- save the widget
- Expected the zoom to button is not visible because there is not event filter applied
- Current the zoom to button is visible even if there is not event filter applied
| @@ -714,13 +715,16 @@ class OpenlayersMap extends React.Component { | |||
| const isDegenerate = bounds[0] === bounds[2] && bounds[1] === bounds[3]; | |||
| // TODO: allow maxZoom to be customizable | |||
| const maxZoom = isDegenerate && isNil(zoomLevel) ? 21 : zoomLevel; | |||
| const mapEl = this.map.getTargetElement(); | |||
| const {top = 0, right = 0, bottom = 0, left = 0} = resolveZoomToExtentPadding(mapEl, padding); | |||
| const paddingValues = [top, right, bottom, left]; | |||
| this.map.getView().fit(bounds, { | |||
| size: this.map.getSize(), | |||
| // mapPaddingSelector returns null while the layout epic is mid-computation | |||
| // (e.g. during a setView swap on projection change). Pass undefined in that | |||
| // case so OL applies its own [0,0,0,0] default - passing null reaches | |||
| // View.fitInternal where padding[1] dereferences and throws. | |||
| ...(padding ? { padding: [padding.top || 0, padding.right || 0, padding.bottom || 0, padding.left || 0] } : {}), | |||
| ...(paddingValues.some(value => value > 0) ? { padding: paddingValues } : {}), | |||
There was a problem hiding this comment.
It's better we don't include Widgets logic inside the Map component, so please restore this file. We should avoid to import this utils and the padding should be resolved at epic level when we are using buildZoomToExtentAction (note buildZoomToExtentAction has a padding argument but never used). If we externalize this to the epic also the Cesium map will get the correct padding without changes on the map engine components
There was a problem hiding this comment.
This would require updating the Cesium map component as well, since it currently doesn't account for padding. I initially thought this was intentional and focused only on the OpenLayers map, but it looks like the padding support may have been missed in the original Cesium implementation. I will update it to handle the padding consistently
| * Values are arrays to support multiple targets per event. | ||
| */ | ||
| export const EVENT_TARGET_MAP = { | ||
| [EVENTS.FILTER_CHANGE]: [TARGET_TYPES.APPLY_FILTER, TARGET_TYPES.APPLY_STYLE, TARGET_TYPES.APPLY_DIMENSION] |
There was a problem hiding this comment.
Do we need to add APPLY_ZOOM_TO event?
There was a problem hiding this comment.
I don't think so. EVENT_TARGET_MAP isn't used anywhere yet
|
@ElenaGallo please test this enhancement on dev, thanks |
3 participants
Description
This PR enhances the dynamic filter widget by adding a new interaction
Zoom Towhich let's the user performautoZoomor manual zoom on the target map. Dashboard: one target per mapPlease check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
What is the current behavior?
What is the new behavior?
Zoom the map to the filtered geometry of connected layers. Supports both manual zoom via a button and automatic zoom when filters change.
Map
zoom-to.mp4
Dashboard
zoom-to-dashboard.mp4
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information