-
Notifications
You must be signed in to change notification settings - Fork 440
Layer Selection Plugin on ArcGIS, WFS & WMS layers #11119
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
base: master
Are you sure you want to change the base?
Conversation
|
@arxitpln thank you so much for your contribution. PR state updated as needed. We will review it soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 ,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| { | ||
| "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin should be available only in contexts so we should remove it from localConfig.json
| { | ||
| "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=="/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we could use react-select instead of custom code, in this way we could remove also the SelectRefContext context provider


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