Layer Selection Plugin on ArcGIS, WFS & WMS layers#11119
Layer Selection Plugin on ArcGIS, WFS & WMS layers#11119arxitpln wants to merge 71 commits intogeosolutions-it:masterfrom
Conversation
|
@arxitpln thank you so much for your contribution. PR state updated as needed. We will review it soon. |
allyoucanmap
left a comment
There was a problem hiding this comment.
@arxitpln Before proceed in depth with the review few initial feedback:
- the selection seems to work only in 2D map (OpenLayers) but not on 3D map (Cesium). I'm seeing usage of drawing actions in this implementation but in newer components we are adopting the usage of drawing components (eg: Annotations)
- It would be better to rename the plugin to
LayersSelection(Select seems too generic) - It would be better to isolate all the LayerSelection components, actions, reducers, epics, utils, ... in the same folder eg:
|__plugins/LayerSelection
|__actions/
|__components/
|__reducers/
|__selectors/
|__utils
|__index.js
- Missing unit tests
- Missing jsDoc
- We need to review and create a specific glyphicon for this plugin
|
Hello @allyoucanmap ,
|
allyoucanmap
left a comment
There was a problem hiding this comment.
Pending issue to address:
- The selection mode does not work in 3D
- The
LayersSelections.jsxplugin should be moved inside theLayersSelectiondirectory introducing an index.js file - missing unit tests
- missing JSDoc with description of cfg available configuration for this plugin
- Inline comments
|
|
||
| .ellipsis-button { | ||
| padding: 2%; | ||
| background-color: lightgray; |
There was a problem hiding this comment.
All the styles related to theme colors should be included in the lesscss files under a including a new file with the name of plugin, in this case layer-selection.less. This is needed to use variables instead of hardcoded value for colors
web/client/configs/localConfig.json
Outdated
| { | ||
| "name": "LayersSelection", | ||
| "cfg": { | ||
| "highlightOptions": { | ||
| "color": "#3388ff", | ||
| "dashArray": "", | ||
| "fillColor": "#3388ff", | ||
| "fillOpacity": 0.2, | ||
| "radius": 4, | ||
| "weight": 4 | ||
| }, | ||
| "queryOptions": { | ||
| "maxCount": -1 | ||
| }, | ||
| "selectTools": [ | ||
| "Point", | ||
| "Line", | ||
| "Circle", | ||
| "Rectangle", | ||
| "Polygon" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
This plugin should be available only in contexts so we should remove it from localConfig.json
web/client/configs/simple.json
Outdated
| { | ||
| "name": "LayersSelection", | ||
| "cfg": { | ||
| "highlightOptions": { | ||
| "color": "#3388ff", | ||
| "dashArray": "", | ||
| "fillColor": "#3388ff", | ||
| "fillOpacity": 0.2, | ||
| "radius": 4, | ||
| "weight": 4 | ||
| }, | ||
| "queryOptions": { | ||
| "maxCount": -1 | ||
| }, | ||
| "selectTools": [ | ||
| "Point", | ||
| "Line", | ||
| "Circle", | ||
| "Rectangle", | ||
| "Polygon" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
This plugin should be available only in contexts so we should remove it from simple.json
| disablePluginIf: "{state('router') && (state('router').endsWith('new') || state('router').includes('newgeostory') || state('router').endsWith('dashboard'))}" | ||
| }, | ||
| reducers: { | ||
| ...controls, |
There was a problem hiding this comment.
A reducer should not be spread. In this case if we want also to ensure controls reducer is included we should review reducers as:
reducers: {
controls,
layersSelection
}And instead of select I propose to change the name of reducer to layersSelection
| * @function | ||
| * @returns {Object} A plugin definition object used by the application to render and control the Select tool. | ||
| */ | ||
| export default createPlugin('Select', { |
There was a problem hiding this comment.
The map viewer remains empty because the configured name here is wrong.
The name should be LayersSelection.
| // eslint-disable-next-line react/jsx-boolean-value | ||
| show={true} |
There was a problem hiding this comment.
The // eslint-disable-next-line react/jsx-boolean-value comment should be removed and show={true} should be just show
| // eslint-disable-next-line react/jsx-boolean-value | ||
| showFullTitle={true} |
There was a problem hiding this comment.
The // eslint-disable-next-line react/jsx-boolean-value comment should be removed and showFullTitle={true} should be just showFullTitle
| // eslint-disable-next-line react/jsx-boolean-value | ||
| draggable={true} |
There was a problem hiding this comment.
The // eslint-disable-next-line react/jsx-boolean-value comment should be removed and draggable={true} should be just draggable
| onClose = {onClose} | ||
| enableFooter={false} | ||
| title={<span className="title-container"> | ||
| <img className="title-icon" alt="icon" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABIAAAASCAYAAABWzo5XAAAAx3pUWHRSYXcgcHJvZmlsZSB0eXBlIGV4aWYAAHjabVBbDsMgDPvnFDtCIAGS49C1k3aDHX+myaq2miXycmtM0vZ5v9JjomRJUrs2a40AMbEyUCg5xh4zyR69KcHl6zwdRMGIkdlbbcePPs+HgKeBqp6E9BnEciVMQl9vQnERT0fTxBpCFkJcnMghMPxZ1Ez7+QnLRleonzSDrfunlC3c3Hrp2N5acQ+XsnFmQmRWN8Dz1MQDhSBOGoa5oWbkAbqHEyzk355+SF8bPFmNwFcwVQAAAYRpQ0NQSUNDIHByb2ZpbGUAAHicfZE9SMNAHMVfP6RSqg52UBHJUJ3soiKOpYpFsFDaCq06mFz6BU0akhQXR8G14ODHYtXBxVlXB1dBEPwAcRecFF2kxP8lhRYxHhz34929x907wNusMsXwxwBFNfV0Ii7k8qtC4BV+BNGPMQyLzNCSmcUsXMfXPTx8vYvyLPdzf44+uWAwwCMQx5imm8QbxLObpsZ5nzjMyqJMfE48qdMFiR+5Ljn8xrlks5dnhvVsep44TCyUuljqYlbWFeIZ4oisqJTvzTksc97irFTrrH1P/sJQQV3JcJ3mKBJYQhIpCJBQRwVVmIjSqpJiIE37cRf/iO1PkUsiVwWMHAuoQYFo+8H/4He3RnF6ykkKxYGeF8v6GAcCu0CrYVnfx5bVOgF8z8CV2vHXmsDcJ+mNjhY5Aga2gYvrjibtAZc7wNCTJuqiLfloeotF4P2MvikPDN4CwTWnt/Y+Th+ALHW1fAMcHAITJcped3l3b3dv/55p9/cDj5pyslRWqj0AAA12aVRYdFhNTDpjb20uYWRvYmUueG1wAAAAAAA8P3hwYWNrZXQgYmVnaW49Iu+7vyIgaWQ9Ilc1TTBNcENlaGlIenJlU3pOVGN6a2M5ZCI/Pgo8eDp4bXBtZXRhIHhtbG5zOng9ImFkb2JlOm5zOm1ldGEvIiB4OnhtcHRrPSJYTVAgQ29yZSA0LjQuMC1FeGl2MiI+CiA8cmRmOlJERiB4bWxuczpyZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1ucyMiPgogIDxyZGY6RGVzY3JpcHRpb24gcmRmOmFib3V0PSIiCiAgICB4bWxuczp4bXBNTT0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wL21tLyIKICAgIHhtbG5zOnN0RXZ0PSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC8xLjAvc1R5cGUvUmVzb3VyY2VFdmVudCMiCiAgICB4bWxuczpkYz0iaHR0cDovL3B1cmwub3JnL2RjL2VsZW1lbnRzLzEuMS8iCiAgICB4bWxuczpHSU1QPSJodHRwOi8vd3d3LmdpbXAub3JnL3htcC8iCiAgICB4bWxuczp0aWZmPSJodHRwOi8vbnMuYWRvYmUuY29tL3RpZmYvMS4wLyIKICAgIHhtbG5zOnhtcD0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wLyIKICAgeG1wTU06RG9jdW1lbnRJRD0iZ2ltcDpkb2NpZDpnaW1wOjRlZTJmMDYyLTc0ZjgtNDdlOC05YzRmLTFkYWM0YThhYTg2ZCIKICAgeG1wTU06SW5zdGFuY2VJRD0ieG1wLmlpZDowOTBmMmVjNi1hMzcxLTQ1YjctOTI0MC0wMmI4Nzk5MTZkOTMiCiAgIHhtcE1NOk9yaWdpbmFsRG9jdW1lbnRJRD0ieG1wLmRpZDpkM2Y0Y2U4ZS04ZmY2LTQxOTUtYjllOS1hMzZiNGY2NWEwMjkiCiAgIGRjOkZvcm1hdD0iaW1hZ2UvcG5nIgogICBHSU1QOkFQST0iMi4wIgogICBHSU1QOlBsYXRmb3JtPSJXaW5kb3dzIgogICBHSU1QOlRpbWVTdGFtcD0iMTc0NDI5NTQ4NzE2MjU4OSIKICAgR0lNUDpWZXJzaW9uPSIyLjEwLjM4IgogICB0aWZmOk9yaWVudGF0aW9uPSIxIgogICB4bXA6Q3JlYXRvclRvb2w9IkdJTVAgMi4xMCIKICAgeG1wOk1ldGFkYXRhRGF0ZT0iMjAyNTowNDoxMFQxNjozMToyNyswMjowMCIKICAgeG1wOk1vZGlmeURhdGU9IjIwMjU6MDQ6MTBUMTY6MzE6MjcrMDI6MDAiPgogICA8eG1wTU06SGlzdG9yeT4KICAgIDxyZGY6U2VxPgogICAgIDxyZGY6bGkKICAgICAgc3RFdnQ6YWN0aW9uPSJzYXZlZCIKICAgICAgc3RFdnQ6Y2hhbmdlZD0iLyIKICAgICAgc3RFdnQ6aW5zdGFuY2VJRD0ieG1wLmlpZDplNDdiZjdkZC00NTMzLTQ1MWItOWFiOC0xNDUxYTk3YmVlMjAiCiAgICAgIHN0RXZ0OnNvZnR3YXJlQWdlbnQ9IkdpbXAgMi4xMCAoV2luZG93cykiCiAgICAgIHN0RXZ0OndoZW49IjIwMjUtMDQtMTBUMTY6MzE6MjciLz4KICAgIDwvcmRmOlNlcT4KICAgPC94bXBNTTpIaXN0b3J5PgogIDwvcmRmOkRlc2NyaXB0aW9uPgogPC9yZGY6UkRGPgo8L3g6eG1wbWV0YT4KICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgICAgICAgICAgICAgICAgIAo8P3hwYWNrZXQgZW5kPSJ3Ij8+XeQ7+AAAAAZiS0dEAP8A/wD/oL2nkwAAAAlwSFlzAAASdAAAEnQB3mYfeAAAAAd0SU1FB+kECg4fGyoDTDYAAACGSURBVDjL7ZTBCcNQDEOVTuARPIo38yjfm2kU5dD8JLQ5/CQUSqnABxv8MAIZVSUzEzZpqaHe3VVVgrufWjzqXw75Ek27M6eLDAHAAz+rv0ef8egtDpl5mJ0R0FqZeTlnK8TMRPIeaEm83P0J64MzbyMzRVL9BUWE0FrrgyHQ3hOSigiR1AwufF67ifcbTQAAAABJRU5ErkJggg=="/> |
| <div className="select-header"> | ||
| <div className="select-button-container"> | ||
| <button className="select-button" onClick={toggleMenu}> | ||
| <span className="select-button-text">{selectedTool ? <><Glyphicon glyph={allTools.find(tool => tool.type === selectedTool.type)?.icon} />{' '}</> : null}<Message msgId={selectedTool?.label ?? "layersSelection.button.chooseGeometry"} /></span> | ||
| <span className="select-button-arrow"><Glyphicon glyph={menuOpen ? 'chevron-up' : 'chevron-down'}/></span> | ||
| </button> | ||
| {menuOpen && ( | ||
| <div className="select-button-menu"> | ||
| {orderedTools.map(tool => <p key={tool.type} onClick={() => clean(tool)}><Glyphicon glyph={tool.icon} />{' '}<Message msgId={tool.label} /></p>)} | ||
| </div> | ||
| )} | ||
| </div> | ||
| <button className="clear-select-button" onClick={clearSelection}> | ||
| <Message msgId="layersSelection.button.clear"/> | ||
| </button> |
There was a problem hiding this comment.
Here we could use react-select instead of custom code, in this way we could remove also the SelectRefContext context provider
…into arxit-select
|
@arxitpln I updated this PRs with following changes:
Please double check and test locally if all applied changes are working as expected, finally to complete the PR merge are missing:
|
|
In order to contribute to the MapStore project, the CLA (Contributor License agreement) should be sent signed to GeoSolutions. Please consult contributing rules at: https://github.com/geosolutions-it/MapStore2/wiki/Contributing-to-MapStore#contributing-code |
|
In order to contribute to the MapStore project, the CLA (Contributor License agreement) should be sent signed to GeoSolutions. Please consult contributing rules at: https://github.com/geosolutions-it/MapStore2/wiki/Contributing-to-MapStore#contributing-code |
|
Dear @allyoucanmap Should we be editing this file to address the issue? Are there any specific requirements we should follow? |
|
Dear @arxitpln , Now there are two folders for this new plugin one LayersSelection and layersSelection, see https://github.com/geosolutions-it/MapStore2/pull/11119/changes
for this problem the docma config is trying to look for configuration of two plugins:
As mentioned in this comment #11119 (comment) I renamed the plugin with L upper case to keep it consistent with other plugins module naming. This commit 5e95477 introduced the duplication, could you please fix it? It would be better to have more coverage for unit tests. the unit tests should cover at least the following files:
|
allyoucanmap
left a comment
There was a problem hiding this comment.
see this comment #11119 (comment)




Description
Plugin which enable for the user to draw a geometry on the map to select all the layer features that cross or intersect the geometry
Please 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?
No Select Plugin
#11076
What is the new behavior?
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information