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

MM-230 Use common panel to display search results and include # of co-ops filtered #246

Merged
merged 11 commits into from
May 10, 2024
2 changes: 1 addition & 1 deletion DEVELOPMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ mykomap's repository:
Install the prerequisites

apt install php php-curl rsync # On Debian
npm run install
npm install

Run the development web server in the background (do this in a separate console):

Expand Down
83 changes: 50 additions & 33 deletions src/map-app/app/map-ui.ts
rogup marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { EventBus } from "../eventbus";
import "./map"; // Seems to be needed to prod the leaflet CSS into loading.
import { SidebarPresenter } from "./presenter/sidebar";
import { PhraseBook } from "../localisations";
import { toString as _toString } from '../utils';
import { toString as _toString, canDisplayExpandedSidebar } from '../utils';
import { Action, AppState, PropEquality, StateManager, TextSearch } from "./state-manager";
import { StateChange } from "../undo-stack";
import { Dictionary } from "../common-types";
Expand Down Expand Up @@ -48,7 +48,8 @@ export class MapUI {
});
};

EventBus.Directory.initiativeClicked.sub(initiative => this.onInitiativeClickedInSidebar(initiative));
EventBus.Map.initiativeClicked.sub(initiative => this.onInitiativeClicked(initiative));
EventBus.Map.clearFiltersAndSearch.sub(() => this.clearFiltersAndSearch());
}

// This inspects the config and constructs an appropriate set of
Expand Down Expand Up @@ -113,8 +114,8 @@ export class MapUI {
this.stateManager.clearPropFilter(filterName);
}

resetSearch(): void {
this.stateManager.reset();
clearFiltersAndSearch(): void {
this.stateManager.clearFiltersAndSearch();
}

/// Returns a list of property values matching the given filter
Expand All @@ -131,36 +132,38 @@ export class MapUI {
throw new Error(`an attempt to add a filter on an unknown propery name '${propName}'`);

const isMulti = propDef.type === 'multi';
if (value === undefined)
if (value === undefined) {
this.stateManager.clearPropFilter(propName);
else
} else {
this.stateManager.propFilter(new PropEquality(propName, value, isMulti));
}

if (canDisplayExpandedSidebar()) // on smaller screens, wait until user clicks Apply Filters
EventBus.Sidebar.showInitiativeList.pub();
}

isSelected(initiative: Initiative): boolean {
// Currently unimplemented - I can see an sea-initiative-active class
// being applied if a test was true, but it makes no sense in the current
// context AFAICT. The test was:
// initiative === this.contextStack.current()?.initiatives[0]

// The main thing is seemed to do is highlight an initiative (or
// initiatives) in the directory pop-out list of initiatives in a
// category.

// For now, just return false always. We may re-implement this later.
return false;
const selectedInitiatives = this.mapPresenter?.getSelectedInitiatives() ?? [];
return selectedInitiatives.includes(initiative);
}

toggleSelectInitiative(initiative: Initiative) {
// Currently unimplemented.
const selectedInitiatives = this.mapPresenter?.getSelectedInitiatives() ?? [];

// EventBus.Markers.needToShowLatestSelection.pub(lastContent.initiatives);
if (selectedInitiatives.includes(initiative)) {
EventBus.Markers.needToShowLatestSelection.pub(selectedInitiatives.filter(i => i !== initiative));
} else {
EventBus.Markers.needToShowLatestSelection.pub([...selectedInitiatives, initiative]);
}
}
Comment on lines 145 to 158
Copy link
Contributor

@wu-lee wu-lee Apr 19, 2024

Choose a reason for hiding this comment

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

Puzzled by this - seems to be re-implementing initiative selection, something which I had thought maybe used to be a thing, long ago but had been unused and probably broken for ages.

Doesn't seem to be related to counting and displaying visible initiatives?

Actually, a lot of the changes in this branch don't appear to be, so general puzzlement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method wasn't implemented, but I needed it in order to highlight the selected initiative in the sidebar.

The previous code for populating the initiative sidebar tried to achieve this by setting the class .sea-initiative-active but this didn't work because the class didn't persist when the sidebar UI was refreshed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm uncertain, but I seem to remember that selection stuff is to do with selecting markers on the map, not selecting sidebar items.

I think here you're just using them to zoom into the marker? Via EventBus.Markers.needToShowLatestSelection?

Not a big deal, just checking one sort of functionality isn't being confused for another and revived to serve a different purpose.

Copy link
Contributor Author

@rogup rogup Apr 19, 2024

Choose a reason for hiding this comment

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

I think because I'm coming from LX, I'm seeing things in a Redux "app state" kind of way.

We do sort of keep a state of the selected markers on the map, within the currentlySelected array (see your comment below). When we click on an initiative in the sidebar, or on a marker directly, it adds the initiative to this array. So I've written this function to expose this state, so that the currently selected initiative is also highlighted in the results pane. It should work however the marker is selected.

Tangential to this point: I think this is actually the most frustrating bit of the UI. The app's state is fragmented and stored all over the code, within different classes. And then if another class wants to access or change the state, it's a nightmare to thread the information through a chain of functions, especially if they're in different parts of the UI. The EventBus broadcasts try to solve this but it basicaly violates the point of OOP, feels quite hacky, and doesn't make the code any clearer.
It's a limitation of OOP that I experienced as a Java Android developer, since it often leads to anti-patterns, unless the code is written in a very strict and methodical way with a clear structure (which this isn't).
If we were to re-write some of the UI code, I think this might actually be the most beneficial change- to move app state to a central store such as Redux, which any part of the UI can hook into. Since we implemented Redux more widely in LX, it has made the code a lot cleaner. I think this is more pressing than changing to something like React. The html structure is already fairly understandable and d3 doesn't seem terrible.

Copy link
Contributor

@wu-lee wu-lee May 2, 2024

Choose a reason for hiding this comment

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

I think something like this would be a first step to anything similar to introducing react anyway.

However, suspect it still won't be easy! There is a lot of stuff, and untangling all the state might not be simple. Therefore I suspect it would need to be further broken down into steps which can be attempted independently...

I've tried to get rid of the EventBus stuff as much as I could and at least make it typesafe (previously it was easy to send malformed messages to misnamed targets, and what the allowed targets was totally opaque and undocumented). I've not entirely succeeded but it's better than it was. What probably needs to be done is to continue with that so that we have a kind if "central source of truth" which behaves more like you get in Redux. But there are other bits of state and coupled logic all over the place, and I don't know how deep those depths go...

Which ultimately means we need to get budget-holder clearance to put our snorkels on and try investigating - but leave a route to bail out if it starts getting too tangled or taking too long.

On the other hand - we might want to look at the bigger picture an see if that goes somewhere we need to be, or if we can instead dredge out the useful bits and retrofit them into a new framework - perhaps if we want to be able to make very big maps? Therefore needs the kind of high-level outcomes planning we scheduled (for today but had to postpone.)



performSearch(text: string) {
console.log("Search submitted: [" + text + "]");
this.stateManager.textSearch(new TextSearch(text));

if (canDisplayExpandedSidebar()) {// on smaller screens, wait until user clicks Apply Filters
EventBus.Sidebar.showInitiativeList.pub();
}
}

// Text to show in the search box
Expand All @@ -182,26 +185,40 @@ export class MapUI {
}

private refreshSidebar() {
this.getSidebarPresenter(this).then((presenter) => presenter.changeSidebar());
this.getSidebarPresenter(this).then((presenter) => {
presenter.changeSidebar();
});
}



private notifyMapNeedsToNeedsToBeZoomedAndPannedOneInitiative(initiative: Initiative) {
const data = EventBus.Map.mkSelectAndZoomData([initiative]);
EventBus.Map.needsToBeZoomedAndPanned.pub(data);
private notifyMapNeedsToNeedsToSelectAndZoomOnInitiative(initiative: Initiative) {
const maxZoom = this.config.getMaxZoomOnOne();
const defaultPos = this.config.getDefaultLatLng();

const data = EventBus.Map.mkSelectAndZoomData([initiative], { maxZoom, defaultPos });
EventBus.Map.selectAndZoomOnInitiative.pub(data);
}

private onInitiativeClickedInSidebar(initiative?: Initiative) {
if (!initiative)
return;
private onInitiativeClicked(initiative?: Initiative) {
console.log('Clicked', initiative);

//this.parent.mapui.stateManager.append(new SearchResults([initiative]));
//console.log(this.parent.mapui.stateManager.current());
if (initiative) {
// Move the window to the right position first
this.notifyMapNeedsToNeedsToSelectAndZoomOnInitiative(initiative);
this.refreshSidebar();
// Populate the sidebar and highlight the intiative in the directory
this.getSidebarPresenter(this).then((presenter) => {
presenter.populateInitiativeSidebar(
initiative,
this.markers.getInitiativeContent(initiative) ?? ''
);
});
}
else {
// User has deselected
EventBus.Markers.needToShowLatestSelection.pub([]);
}

this.notifyMapNeedsToNeedsToBeZoomedAndPannedOneInitiative(initiative);
this.refreshSidebar();
EventBus.Initiative.searchedInitiativeClicked.pub(initiative);
}

currentItem() {
Expand Down
13 changes: 8 additions & 5 deletions src/map-app/app/presenter/map.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { MapUI } from '../map-ui';

export class MapPresenter extends BasePresenter {
readonly view: MapView;
private previouslySelected: Initiative[] = [];
private currentlySelected: Initiative[] = [];

rogup marked this conversation as resolved.
Show resolved Hide resolved
constructor(readonly mapUI: MapUI) {
super();
Expand All @@ -35,7 +35,7 @@ export class MapPresenter extends BasePresenter {
EventBus.Initiatives.loadStarted.sub(() => this.onInitiativeLoadMessage());
EventBus.Initiatives.loadFailed.sub(error => this.onInitiativeLoadMessage(error));

EventBus.Markers.needToShowLatestSelection.sub(initiative => this.onMarkersNeedToShowLatestSelection(initiative));
EventBus.Markers.needToShowLatestSelection.sub(initiatives => this.onMarkersNeedToShowLatestSelection(initiatives));
EventBus.Map.needsToBeZoomedAndPanned.sub(data => this.onMapNeedsToBeZoomedAndPanned(data));
EventBus.Map.needToShowInitiativeTooltip.sub(initiative => this.onNeedToShowInitiativeTooltip(initiative));
EventBus.Map.needToHideInitiativeTooltip.sub(initiative => this.onNeedToHideInitiativeTooltip(initiative));
Expand All @@ -49,7 +49,7 @@ export class MapPresenter extends BasePresenter {
}

onInitiativeClicked() {
EventBus.Directory.initiativeClicked.pub(undefined);
EventBus.Map.initiativeClicked.pub(undefined);
}

onLoad() {
Expand Down Expand Up @@ -96,11 +96,11 @@ export class MapPresenter extends BasePresenter {
}

onMarkersNeedToShowLatestSelection(selected: Initiative[]) {
this.previouslySelected.forEach((initiative) => {
this.currentlySelected.forEach((initiative) => {
this.mapUI.markers.setUnselected(initiative);
});

this.previouslySelected = selected;
this.currentlySelected = selected;

//zoom in and then select
selected.forEach((initiative) => {
Expand Down Expand Up @@ -144,4 +144,7 @@ export class MapPresenter extends BasePresenter {
this.view.selectAndZoomOnInitiative(data);
}

getSelectedInitiatives(): Initiative[] {
return this.currentlySelected;
}
}
23 changes: 19 additions & 4 deletions src/map-app/app/presenter/sidebar.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Dictionary } from '../../common-types';
import { EventBus } from '../../eventbus';
import { MapUI } from '../map-ui';
import { Initiative } from '../model/initiative';
import { SidebarView } from '../view/sidebar';
import { BasePresenter } from './base';
import { AboutSidebarPresenter } from './sidebar/about';
Expand Down Expand Up @@ -63,7 +64,7 @@ export class SidebarPresenter extends BasePresenter {
if (name !== undefined) {
// If name is set, change the current sidebar and then refresh
this.sidebarName = name;
this.children[this.sidebarName]?.refreshView();
this.children[this.sidebarName]?.refreshView(true);
}
else {
// Just refresh the currently showing sidebar.
Expand All @@ -76,11 +77,13 @@ export class SidebarPresenter extends BasePresenter {
return; // No sidebars? Can't do anything.
}

this.children[this.sidebarName]?.refreshView();
this.children[this.sidebarName]?.refreshView(false);
}
}

showSidebar() {
// Refresh the view before showing
this.children[this.sidebarName ?? 'undefined']?.refreshView(true);
this.view.showSidebar();
}

Expand All @@ -97,6 +100,14 @@ export class SidebarPresenter extends BasePresenter {
this.view.hideInitiativeSidebar();
}

populateInitiativeSidebar(initiative: Initiative, initiativeContent: string) {
this.view.populateInitiativeSidebar(initiative, initiativeContent);
}

showInitiativeList() {
this.view.showInitiativeList();
}

hideInitiativeList() {
this.view.hideInitiativeList();
}
Expand All @@ -112,7 +123,7 @@ export class SidebarPresenter extends BasePresenter {
});
EventBus.Sidebar.showDirectory.sub(() => {
this.changeSidebar("directory");
this.view.showInitiativeList();
this.showSidebar();
});
EventBus.Sidebar.showDatasets.sub(() => {
this.changeSidebar("datasets");
Expand All @@ -121,8 +132,12 @@ export class SidebarPresenter extends BasePresenter {
EventBus.Sidebar.showSidebar.sub(() => this.showSidebar());
EventBus.Sidebar.hideSidebar.sub(() => this.hideSidebar());
EventBus.Sidebar.hideInitiativeSidebar.sub(() => this.hideInitiativeSidebar());
EventBus.Sidebar.showInitiativeList.sub(() => this.showInitiativeList());
EventBus.Sidebar.hideInitiativeList.sub(() => this.hideInitiativeList());
EventBus.Initiatives.reset.sub(() => this.changeSidebar());
EventBus.Initiatives.reset.sub(() => {
this.changeSidebar();
this.hideInitiativeList();
});
EventBus.Initiatives.loadComplete.sub(() => this.changeSidebar());
}

Expand Down
39 changes: 32 additions & 7 deletions src/map-app/app/presenter/sidebar/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { EventBus } from '../../../eventbus';
import { BasePresenter }from '../base';
import { BaseSidebarView } from '../../view/sidebar/base';
import { SidebarPresenter } from '../sidebar';
import { Initiative } from '../../model/initiative';
import { canDisplayExpandedSidebar } from '../../../utils';

export interface NavigationCallback {
disabled: boolean;
Expand All @@ -16,10 +18,16 @@ export abstract class BaseSidebarPresenter extends BasePresenter {
super();
}

// If the sidebar wants to do something more than to get its view to refresh when the history buttons have been used, then
// it should override this definition with its own:
historyButtonsUsed(): void {
this.view.refresh();
/**
* If the sidebar wants to do something more than to get its view to refresh when the history
* buttons have been used, then it should override this definition with its own.
*/
historyButtonsUsed(): void {
this.view.refresh(false);

// Show the results pane again, since there have been changes
if (canDisplayExpandedSidebar()) // on smaller screens, wait until user clicks Apply Filters
EventBus.Sidebar.showInitiativeList.pub();
}

deselectInitiatives(): void {
Expand Down Expand Up @@ -48,9 +56,26 @@ export abstract class BaseSidebarPresenter extends BasePresenter {
this.historyButtonsUsed();
}

/// Refreshes the view
refreshView() {
this.view.refresh();
/**
* Refreshes the sidebar view
*
* @param changed true if we changed to this sidebar, false if it was already showing and we're
* just refreshing it.
*/
refreshView(changed: boolean) {
this.view.refresh(changed);
}

onInitiativeClicked(initiative: Initiative): void {
EventBus.Map.initiativeClicked.pub(initiative);
}

onInitiativeMouseoverInSidebar(initiative: Initiative): void {
EventBus.Map.needToShowInitiativeTooltip.pub(initiative);
}

onInitiativeMouseoutInSidebar(initiative: Initiative): void {
EventBus.Map.needToHideInitiativeTooltip.pub(initiative);
}
}

Expand Down
Loading
Loading