#10545: Option to disable identify popup in case of no results #10557
+122
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… results Description: - handle adding an option called 'hidePopupIfNoResults' to hide the identify popup - add unit tests - add jsdoc
mahmoudadel54
added
enhancement
BackportNeeded
6 tasks
MV88
requested changes
Sep 20, 2024
| @@ -91,6 +93,9 @@ export const identifyLifecycle = compose( | |||
| showAllResponses, | |||
| highlight: pluginCfg?.highlightEnabledFromTheStart || false | |||
| }); | |||
| if (hidePopupIfNoResults) { | |||
| enableHideEmptyPopupOption(true); | |||
There was a problem hiding this comment.
can't you just use the hidePopupIfNoResults prop instead of calling enableHideEmptyPopupOption and updating state?
Comment on lines
+110
to
+126
| (state) => state.mapPopups && state.mapPopups.hideEmptyPopupOption || false, | ||
| isMouseMoveActiveSelector, | ||
| mapInfoRequestsSelector, | ||
| responsesSelector, | ||
| (popups, hideEmptyPopupOption, isMouseMoveActive, mapInfoRequests, mapInfoResponses) => { | ||
| // create a flag for hide the identify popup in case no results in hover mode | ||
| let identifyPopupHidden = false; | ||
| if (isMouseMoveActive && mapInfoRequests?.length && mapInfoResponses?.length && hideEmptyPopupOption) { | ||
| const format = getDefaultInfoFormatValue(); | ||
| const invalidResponses = getValidator(format).getNoValidResponses(mapInfoResponses); | ||
| const emptyResponses = mapInfoRequests?.length === invalidResponses?.length; | ||
| const missingResponses = (mapInfoRequests || []).length - (mapInfoResponses || []).length; | ||
| identifyPopupHidden = missingResponses === 0 && emptyResponses && isMouseMoveActive && hideEmptyPopupOption; | ||
| } | ||
| return { | ||
| popups, | ||
| identifyPopupHidden |
There was a problem hiding this comment.
I have also some doubts here tbh, i will deeply check this
| (popups) => ({ | ||
| popups | ||
| })), { | ||
| (state) => state.mapPopups && state.mapPopups.hideEmptyPopupOption || false, |
There was a problem hiding this comment.
a dedicated selector must be created when you add a new one
Comment on lines
+71
to
+79
| const {identifyPopupHidden} = this.props; | ||
| if (identifyPopupHidden) { | ||
| (this._popups || []).forEach(({popup}) => { | ||
| popup.off('remove', this.popupClose); | ||
| popup && this.props.map.removeLayer(popup); | ||
| }); | ||
| return []; | ||
| } | ||
| return this.preparePopups() |
There was a problem hiding this comment.
i have some doubts about this implementation too.
| @@ -59,6 +61,14 @@ export default class PopupSupport extends React.Component { | |||
| this.props.onPopupClose(id); | |||
| } | |||
| renderPopups = () => { | |||
| const {identifyPopupHidden, map} = this.props; | |||
| if (identifyPopupHidden) { | |||
There was a problem hiding this comment.
maybe the flag hidePopupIfNoResults could have been used to filter out elements?
i have to debug this tbh
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In this PR, it is implemented adding an option for identify plugin called 'hidePopupIfNoResults' = true to handle hide identify popup in case of no results for 'openlayers' and 'leaflet' for 2D maps.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
#10545
What is the current behavior?
#10545
What is the new behavior?
If 'hidePopupIfNoResults' is added to cfg in identify plugin by true ---> the identify popup will be hidden if there is no results.
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information
To test it, for desktop add 'hidePopupIfNoResults' with true for cfg in identify plugin.
For mobile, you need to add the identify plugin option 'hidePopupIfNoResults' with true for cfg and edit ---> DEFAULT_VISUALIZATION_MODES_CONFIG.[VisualizationModes._2D].mobile to
MapLibraries.LEAFLETin this part belowMapStore2/web/client/utils/MapTypeUtils.js
Lines 27 to 36 in 6c8e852