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

Dynamic flatmap markers #102

Merged
merged 23 commits into from
Jul 22, 2022
Merged

Conversation

Tehsurfer
Copy link
Contributor

@Tehsurfer Tehsurfer commented Mar 17, 2022

This was referenced Jul 15, 2022
Copy link
Member

@alan-wu alan-wu left a comment

Choose a reason for hiding this comment

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

I have commented on several places.
One minor thing is semi-colon. Please add them in for consistency please.

} else if (action.type == "Facet") {
const speciesFacets = [];
} else if (action.type == "Facet") {
console.log('action', action)
Copy link
Member

Choose a reason for hiding this comment

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

Remove the console.log please.

store.state.settings.facets.species.forEach(e => {
speciesFacets.push({facet: e, term:'species'});
facets.push({facet: e, term:'Species', facetPropPath: 'anatomy.organ.name'});
Copy link
Member

Choose a reason for hiding this comment

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

Is the facetPropPath correct for species?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah you are right

speciesFacets.push({facet: "show all", term:'species'});
this.$refs.sideBar.addFilter({facet: action.label, term:'Anatomical structure', facetPropPath: 'anatomy.organ.name'});
if (facets.length == 0)
facets.push({facet: "Show All", term:'Species', facetPropPath: 'anatomy.organ.name'});
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

const speciesFacets = [];
} else if (action.type == "Facet") {
console.log('action', action)
this.$refs.sideBar.addFilter(action);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having two separate blocks, Can this be replaced by the Facets block instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the Facet action adds on facet to the current facets, as the Facets action clears all facets and sets new ones. I think we need boths

@@ -137,6 +139,10 @@ export default {
if (data && (data.type == "filter-update")) {
store.commit("settings/updateFacets", data.value);
}
if (data && data.type == "keyword-update") {
console.log('starting keyword update with: ', data.value)
Copy link
Member

Choose a reason for hiding this comment

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

Remove the console.log

@@ -74,6 +76,7 @@ export default {
payload: payload,
type: this.entry.type,
};
this.handleSyncPanZoomEvent(result)
Copy link
Member

Choose a reason for hiding this comment

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

This should call this.flatmapMarkerZoomUpdate directly. The handleSyncPanZoomEvent will prevent any update on the marker if the mouse cursor is on the flatmap.

returnedAction.type = "Facet";
returnedAction.label = this.idNamePair[resource.feature.models];
let label = this.idNamePair[resource.feature.models]
window.label = label
Copy link
Member

Choose a reason for hiding this comment

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

Remove all the temporary objects from window please.

} else {
// Facet search on anatomy if it is not a keyword search
returnedAction = {
type: "Facet",
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this so that we can use the Facets block as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we want markers to add a filter to the current filter in the sidebar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(which is what the 'Facet' action does. 'Facets' clears and sets a what array of facets is given)

// Facet search on anatomy if it is not a keyword search
returnedAction = {
type: "Facet",
facet: label,
Copy link
Member

Choose a reason for hiding this comment

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

label is undefined here, I used to call this.idNamePair[resource.feature.models] to get the label instead.

}
},
flatmapMarkerZoomUpdate(){
let flatmapImp = this.getFlatmapImp()
Copy link
Member

Choose a reason for hiding this comment

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

Calling the clearMarkers and addMarker at every zoom event can be intensive. Can we store the current zoom level and only update the markers when the zoom level has changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good idea

 - Add semicolons
 - Remove remaining debug vars
 - Switch marker update so it only happens on zoom level change
 - Add an event for searches to update markers
this.zoomLevel = currentZoom;
this.flatmapMarkerZoomUpdate();
}
this.handleSyncPanZoomEvent(result);
Copy link
Member

Choose a reason for hiding this comment

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

Do not call handleSyncPanZoomEvent here please, that method should be called only when it is triggered by external callback.

@@ -107,7 +119,6 @@ export default {
.getCurrentFlatmap()
.mapImp.panZoomTo(origin, [sW, sH]);
}
this.flatmapMarkerZoomUpdate()
Copy link
Member

Choose a reason for hiding this comment

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

Keep this method, this method gets called by synchronised zoom event. The markers should be updated.

@alan-wu alan-wu self-requested a review July 22, 2022 00:55
@alan-wu alan-wu merged commit 7f58d46 into ABI-Software:main Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants