PB-1068: Add CTRL + Click to Toggle Selected Features#1357
Conversation
web-mapviewer
|
||||||||||||||||||||||||||||
| Project |
web-mapviewer
|
| Branch Review |
feat-pb-1068-ctrl-click
|
| Run status |
|
| Run duration | 05m 16s |
| Commit |
|
| Committer | Pascal Barth |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
20
|
Skipped
|
0
|
Passing
|
253
|
| View all changes introduced in this branch ↗︎ | |
sommerfe
left a comment
There was a problem hiding this comment.
LGTM
just a small comment, is it possible to not show the drag box when only ctrl+click on a feature and only show the drag box when ctrl+click+drag?
pakb
left a comment
There was a problem hiding this comment.
Would be nice to have an e2e test that checks for this new feature support
packages/mapviewer/src/modules/map/components/openlayers/utils/useDragBoxSelect.composable.js
Show resolved
Hide resolved
4c35a3b to
5a89172
Compare
packages/mapviewer/src/modules/map/components/openlayers/utils/useDragBoxSelect.composable.js
Outdated
Show resolved
Hide resolved
packages/mapviewer/tests/cypress/tests-e2e/featureSelection.cy.js
Outdated
Show resolved
Hide resolved
packages/mapviewer/tests/cypress/tests-e2e/featureSelection.cy.js
Outdated
Show resolved
Hide resolved
f62aead to
ee4cbb8
Compare
|
The test was failing over and over, I need to the update from develop branch to fix it. |
pakb
left a comment
There was a problem hiding this comment.
Can you make it so that the selection box isn't reset when we de-select one element from it?
ab18b95 to
412eab5
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds support for CTRL+click to toggle feature selection and visual feedback via a selection rectangle.
- Introduce a new IdentifyMode (NEW vs TOGGLE) and CTRL click type to dispatch toggle vs replace identify operations
- Render a red rectangle around box selections and expose its visibility and coordinates in the store
- Update e2e tests, map interaction composables, and z-index calculations to cover the new behavior
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/mapviewer/tests/cypress/tests-e2e/featureSelection.cy.js | Added clickOnMap helper and a new test for CTRL+click toggling |
| packages/mapviewer/tests/cypress/fixtures/import-tool/4-points.kml | Added KML fixture for toggle selection test |
| packages/mapviewer/src/store/plugins/click-on-map-management.plugin.js | Extended click plugin to detect CTRL clicks and set IdentifyMode |
| packages/mapviewer/src/store/modules/map.store.js | Introduced CTRL_LEFT_SINGLECLICK click type |
| packages/mapviewer/src/store/modules/features.store.js | Added IdentifyMode enum, toggle logic, and rectangle state |
| packages/mapviewer/src/modules/map/components/openlayers/utils/useMapInteractions.composable.js | Use platformModifierKeyOnly to flag CTRL clicks |
| packages/mapviewer/src/modules/map/components/openlayers/utils/useDragBoxSelect.composable.js | Enforce minimum drag distance and dispatch rectangle coords |
| packages/mapviewer/src/modules/map/components/openlayers/OpenLayersSelectionRectangle.vue | New component to draw the selection rectangle |
| packages/mapviewer/src/modules/map/components/openlayers/OpenLayersMap.vue | Conditionally include the selection rectangle overlay |
| packages/mapviewer/src/modules/map/components/common/z-index.composable.js | Compute a z-index for the selection rectangle layer |
| packages/mapviewer/src/config/map.config.js | Defined DEFAULT_FEATURE_IDENTIFICATION_TOLERANCE |
| packages/mapviewer/src/api/features/features.api.js | Removed hardcoded tolerance and imported from config |
Comments suppressed due to low confidence (2)
packages/mapviewer/src/modules/map/components/openlayers/utils/useMapInteractions.composable.js:2
- Missing imports for ClickType and ClickInfo, which are used later in the file. Please add:
import { ClickType, ClickInfo } from '@/store/modules/map.store'.
import { altKeyOnly, platformModifierKeyOnly, primaryAction } from 'ol/events/condition'
packages/mapviewer/src/modules/map/components/openlayers/utils/useMapInteractions.composable.js:2
- [nitpick]
altKeyOnlyis imported but not used in this file. Consider removing it to clean up unused imports.
import { altKeyOnly, platformModifierKeyOnly, primaryAction } from 'ol/events/condition'
packages/mapviewer/src/modules/map/components/openlayers/utils/useDragBoxSelect.composable.js
Outdated
Show resolved
Hide resolved
|
@pakb done, I use the same approach as in the layer extents to keep showing the box. It's actually handled internally by OpenLayers. |
pakb
left a comment
There was a problem hiding this comment.
I'd change a bit the store logic here.
Instead of storing a flag, telling if the selection extent should be shown (and used only in one edge case), I'd store the selection extent itself (derived from the clickInfo).
If the extent is defined, we show it, also in other circumstances (not only when we toggle features off)
A little thing that could be also changed : if we toggle (CTRL+click) on a selected feature, any adjacent feature (or underlying feature) should not be toggled on. If we toggle something off, we are are in "Toggle off" mode. I find it a bit confusing when you CTRL click on the map, with only one of two stacked features selected, and the other one is then toggle on. It feels like the app didn't do anything
but how do we differentiate if the user want to select the unselected feature? I mean the reverse case, when the user want to select a feature, the selected adjacent feature will be unselected. actually, is it really possible? since normally, we select all features in the clicked area or stacked. |
f3beb8d to
9144f9f
Compare
Refines the CTRL+Click feature selection logic to prevent user confusion when toggling features. Specifically, it ensures that adding new features via CTRL+Click does not occur immediately after deselecting an existing feature at the same location, thus avoiding the impression that the deselect action had no effect.
Test link