Skip to content
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

#10545: Option to disable identify popup in case of no results #10557

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions web/client/actions/__tests__/mapPopups-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,9 @@ describe('test map popups action creators', () => {
const action = POPUP.cleanPopups();
expect(action.type).toEqual(POPUP.CLEAN_MAP_POPUPS);
});
it('enable hide empty popup option', () => {
const action = POPUP.enableHideEmptyPopupOption();
expect(action.type).toEqual(POPUP.ENABLE_HIDE_EMPTY_POPUP);
});
});

5 changes: 5 additions & 0 deletions web/client/actions/mapPopups.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
export const ADD_MAP_POPUP = 'MAP:ADD_POPUP';
export const REMOVE_MAP_POPUP = 'MAP:REMOVE_POPUP';
export const CLEAN_MAP_POPUPS = 'MAP:CLEAN_POPUPS';
export const ENABLE_HIDE_EMPTY_POPUP = 'MAP:ENABLE_HIDE_EMPTY_POPUP';

export const addPopup = (id, options, single = true) => ({
type: ADD_MAP_POPUP,
Expand All @@ -26,3 +27,7 @@ export const removePopup = (id) => ({
export const cleanPopups = () => ({
type: CLEAN_MAP_POPUPS
});

export const enableHideEmptyPopupOption = () => ({
type: ENABLE_HIDE_EMPTY_POPUP
});
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,18 @@ describe("test identify enhancers", () => {
);
expect(spyIdentifyIsMounted.calls.length).toEqual(1);
});
it("test identifyLifecycle component for call enableHideEmptyPopupOption if hidePopupIfNoResults prop = true", () => {
const Component = identifyLifecycle(() => <div id="test-component"></div>);
const testHandlers = {
enableHideEmptyPopupOption: () => {}
};
const spyEnableHideEmptyPopupOption = expect.spyOn(testHandlers, 'enableHideEmptyPopupOption');
ReactDOM.render(
<Component enabled responses={[{}]} hidePopupIfNoResults enableHideEmptyPopupOption={testHandlers.enableHideEmptyPopupOption}/>,
document.getElementById("container")
);
expect(spyEnableHideEmptyPopupOption.calls.length).toEqual(1);
});
it("Identify should run when enabled prop is true and showInMapPopup prop is false", () => {
let run = sampleComponentDidMount({enabled: true, showInMapPopup: false});
expect(run.checkIdentifyIsMounted).toBe(true);
Expand Down
7 changes: 6 additions & 1 deletion web/client/components/data/identify/enhancers/identify.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ export const identifyLifecycle = compose(
setShowInMapPopup = () => {},
checkIdentifyIsMounted = () => {},
onInitPlugin = () => {},
pluginCfg = {}
pluginCfg = {},
enableHideEmptyPopupOption = () => {},
hidePopupIfNoResults = false
} = this.props;

// Initialize plugin configuration
Expand All @@ -91,6 +93,9 @@ export const identifyLifecycle = compose(
showAllResponses,
highlight: pluginCfg?.highlightEnabledFromTheStart || false
});
if (hidePopupIfNoResults) {
enableHideEmptyPopupOption(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you just use the hidePopupIfNoResults prop instead of calling enableHideEmptyPopupOption and updating state?

}
if (enabled || showInMapPopup) {
changeMousePointer('pointer');
checkIdentifyIsMounted(true);
Expand Down
16 changes: 13 additions & 3 deletions web/client/components/map/leaflet/PopupSupport.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,13 @@ export default class PopupSupport extends React.Component {
static propTypes = {
map: PropTypes.object,
popups: PropTypes.arrayOf(PropTypes.object),
identifyPopupHidden: PropTypes.bool,
onPopupClose: PropTypes.func
}

static defaultProps = {
popups: [],
identifyPopupHidden: false,
onPopupClose: () => {}
}
UNSAFE_componentWillMount() {
Expand All @@ -50,8 +52,8 @@ export default class PopupSupport extends React.Component {
this.props.map.on('resize', this.updatePopup);
}
}
shouldComponentUpdate({popups}) {
return popups !== this.props.popups;
shouldComponentUpdate({popups, identifyPopupHidden}) {
return popups !== this.props.popups || identifyPopupHidden !== this.props.identifyPopupHidden;
}
componentWillUnmount() {
// Clean old popups without throwing event
Expand All @@ -66,7 +68,15 @@ export default class PopupSupport extends React.Component {
}
}
renderPopups() {
return this.preparePopups()
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()
Comment on lines +71 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have some doubts about this implementation too.

.filter(({component}) => !!component)
.map(({popup, props = {}, component, id}) => {
const context = popup.getContent();
Expand Down
17 changes: 17 additions & 0 deletions web/client/components/map/leaflet/__tests__/PopupSupport-test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -162,5 +162,22 @@ describe('Leaflet PopupSupport', () => {
const close = document.querySelector('.leaflet-popup-close-button');
close.click();
});
it('test hiding popup if no identify results in hover mode', () => {
const popups = [
{
id: 'test',
component: () => (<div style={{width: 300, height: 100}} ></div>),
position: { coordinates: [0, 0]},
autoPan: false
}

];
const component1 = renderPopups({ popups, identifyPopupHidden: true });
expect(component1).toExist();
expect(document.querySelector('.leaflet-popup')).toNotExist();
const component2 = renderPopups({ popups, identifyPopupHidden: false });
expect(component2).toExist();
expect(document.querySelector('.leaflet-popup')).toExist();
});
});

14 changes: 12 additions & 2 deletions web/client/components/map/openlayers/PopupSupport.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,13 @@ export default class PopupSupport extends React.Component {
static propTypes = {
map: PropTypes.object,
popups: PropTypes.arrayOf(PropTypes.object),
identifyPopupHidden: PropTypes.bool,
onPopupClose: PropTypes.func
}

static defaultProps = {
popups: [],
identifyPopupHidden: false,
onPopupClose: () => {}
}
UNSAFE_componentWillMount() {
Expand All @@ -47,8 +49,8 @@ export default class PopupSupport extends React.Component {
this.props.map.getOverlayContainerStopEvent().addEventListener('pointermove', this.stopPropagationOnPointerMove);
}
}
shouldComponentUpdate({popups}) {
return popups !== this.props.popups;
shouldComponentUpdate({popups, identifyPopupHidden}) {
return popups !== this.props.popups || identifyPopupHidden !== this.props.identifyPopupHidden;
}
componentWillUnmount() {
if (this.props.map) {
Expand All @@ -59,6 +61,14 @@ export default class PopupSupport extends React.Component {
this.props.onPopupClose(id);
}
renderPopups = () => {
const {identifyPopupHidden, map} = this.props;
if (identifyPopupHidden) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the flag hidePopupIfNoResults could have been used to filter out elements?
i have to debug this tbh

(this._popups || []).forEach(({popup, observer}) => {
!!observer && observer.disconnect();
!!popup && map.removeOverlay(popup);
});
return [];
}
return this.preparePopups().map(({ popup, id, component, content, props, containerStyle}) => {
const context = popup.getElement();
const PopupContent = isString(component) && popupsComponents[component] || component;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,22 @@ describe('Openlayers PopupSupport', () => {
expect(document.querySelector('#test-html-map-popup')).toExist();
expect(document.querySelector('#test-component-map-popup')).toExist();
});
it('test hiding popup if no identify results in hover mode', () => {
const popups = [
{
id: 'test',
component: () => (<div style={{width: 300, height: 100}} ></div>),
position: { coordinates: [0, 0]},
autoPan: false
}

];
const component1 = renderPopups({ popups, identifyPopupHidden: true });
expect(component1).toExist();
expect(document.querySelector('.map-popup-ol')).toNotExist();
const component2 = renderPopups({ popups, identifyPopupHidden: false });
expect(component2).toExist();
expect(document.querySelector('.map-popup-ol')).toExist();
});
});

5 changes: 4 additions & 1 deletion web/client/plugins/Identify.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
checkIdentifyIsMounted,
onInitPlugin
} from '../actions/mapInfo';
import { enableHideEmptyPopupOption } from '../actions/mapPopups';
import DefaultViewerComp from '../components/data/identify/DefaultViewer';
import { defaultViewerDefaultProps, defaultViewerHandlers } from '../components/data/identify/enhancers/defaultViewer';
import { identifyLifecycle } from '../components/data/identify/enhancers/identify';
Expand Down Expand Up @@ -196,6 +197,7 @@ const identifyDefaultProps = defaultProps({
* @prop cfg.dock {bool} true shows dock panel, false shows modal
* @prop cfg.draggable {boolean} draggable info window, when modal
* @prop cfg.showHighlightFeatureButton {boolean} show the highlight feature button if the interrogation returned valid features (openlayers only)
* @prop cfg.hidePopupIfNoResults {boolean} hide/show the identify popup in case of no results
* @prop cfg.highlightEnabledFromTheStart {boolean} the highlight feature button will be activated by default if true
* @prop cfg.viewerOptions.container {expression} the container of the viewer, expression from the context
* @prop cfg.viewerOptions.header {expression} the header of the viewer, expression from the context{expression}
Expand Down Expand Up @@ -265,7 +267,8 @@ const IdentifyPlugin = compose(
identifyIndex,
defaultViewerHandlers,
connect(() => ({}), {
setShowInMapPopup
setShowInMapPopup,
enableHideEmptyPopupOption
}),
identifyLifecycle
)(IdentifyContainer);
Expand Down
25 changes: 22 additions & 3 deletions web/client/plugins/map/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import { projectionDefsSelector, isMouseMoveActiveSelector } from '../../selecto
import {
snappingLayerSelector
} from "../../selectors/draw";
import { getDefaultInfoFormatValue, getValidator } from '../../utils/MapInfoUtils';
import { mapInfoRequestsSelector, responsesSelector } from '../../selectors/mapInfo';

const Empty = () => { return <span/>; };

Expand Down Expand Up @@ -105,9 +107,26 @@ const pluginsCreator = (mapType, actions) => {
const PopupSupport = connect(
createSelector(
(state) => state.mapPopups && state.mapPopups.popups || EMPTY_POPUPS,
(popups) => ({
popups
})), {
(state) => state.mapPopups && state.mapPopups.hideEmptyPopupOption || false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a dedicated selector must be created when you add a new one

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
Comment on lines +110 to +126
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also some doubts here tbh, i will deeply check this

};
}
), {
onPopupClose: removePopup
}
)(components.PopupSupport || Empty);
Expand Down
6 changes: 5 additions & 1 deletion web/client/reducers/__tests__/mapPopups-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,9 @@ describe('mapPopups reducer', () => {
expect(state.popups).toExist();
expect(state.popups.length).toBe(0);
});

it('ENABLE_HIDE_EMPTY_POPUP ', () => {
const state = reducer(initialState, ACTIONS.enableHideEmptyPopupOption());
expect(state.popups).toExist();
expect(state.hideEmptyPopupOption).toEqual(true);
});
});
7 changes: 6 additions & 1 deletion web/client/reducers/mapPopups.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* LICENSE file in the root directory of this source tree.
*/

import {ADD_MAP_POPUP, REMOVE_MAP_POPUP, CLEAN_MAP_POPUPS} from '../actions/mapPopups';
import {ADD_MAP_POPUP, REMOVE_MAP_POPUP, CLEAN_MAP_POPUPS, ENABLE_HIDE_EMPTY_POPUP} from '../actions/mapPopups';
import {arrayDelete} from '../utils/ImmutableUtils';

const initialState = {popups: []};
Expand All @@ -23,6 +23,11 @@ export default function(state = initialState, action) {
case CLEAN_MAP_POPUPS: {
return {...state, popups: []};
}
case ENABLE_HIDE_EMPTY_POPUP: {
return {
...state, hideEmptyPopupOption: true
};
}
default:
return state;
}
Expand Down
Loading