Closed moontrip closed 7 months ago
Looking great! Is it relatively easy to have the cancellation of the selection respond to a change in the geohash level (it seems to be leaflet zoom level at the moment)?
Looking great! Is it relatively easy to have the cancellation of the selection respond to a change in the geohash level (it seems to be leaflet zoom level at the moment)?
@bobular Thank you for your review/tests 👍 Your suggestion seems to be not that easy but will check it out when examining panning behaviors that are also related to it.
Update:
To-do:
@bobular I think that I addressed all your suggestions, including panning behavior and geohash level-based, not zoom level-based behavior. The only thing left is to check this at SAM. I will branch out from this PR to test with SAM.
Hi @bobular 👋 Thank you for your review and sharing your thoughts 👍 I will reply them one-by-one later.
@bobular I left my thought next to your suggestions/questions starting with the suffix, DK: (bold style)
Assuming that we don't need to include marker data in
selectedMarkers
(which was not requested as part of this ticket), I would like to avoid having to retro-fit every marker type (DonutMarker.tsx, ChartMarker.tsx, BubbleMarker.tsx) with selection handling code.I would imagine that
BoundsDriftMarker
could handle the adding/removing of thehighlighted-marker
class all by itself, and thatBoundsDriftMarker
should have two optional callback propsonSelected
andonUnselected
that expect the marker ID as an argument.These callbacks could be added in
MapVEuMap
where the management of theselectedMarkers
array should also be done:const finalMarkers = useMemo(() => { return markers.map((marker) => cloneElement(marker, { showPopup: true })); }, [markers]);
Note that @dmfalke's refactoring (https://github.com/VEuPathDB/web-monorepo/tree/map-collection-variable-map-type--map-type-refactor) is moving a lot of marker logic around.
In the refactored code, I think the
selectedMarkers
andonSelectedMarkersChange
props would belong toSemanticMarkers
(and thefinalMarkers
map bit would go in there too (where it has been renamedrefinedMarkers
)).The goal would be not needing to retrofit the old stories.
But first, convince me that additional data (not just the marker/geohash ID) is necessary! ;-)
DK: As you can see my first commit in this PR, https://github.com/VEuPathDB/web-monorepo/pull/513/commits/36e38093fa2f1220b8edfaf3c1b43e0c5263f854, I didn't change much at marker components: they only had selectedMarkers and setSelectedMarkers to be passed to BoundsDriftMarker.
It has the following two-folds why marker component has been changed since then: a) to add data (and other info) into the selectedMarkers as you mentioned; b) however, the other important reason is to handle panning behavior as you requested, "any marker that "falls off" the viewport while panning the map becomes unselected." Basically, map panning leads to new data request and accordingly marker-related HTML codes are regenerated. In view of the way how to make highlight marker, which is based on HTML Class & CSS, HTML Class(es) concerning the selected marker(s) (i.e., highlight-donutmarker, etc.) is lost whenever map panning happens. The only way I found to overcome this issue is to check panning event, and update the highlight HTML Class again based on selectedMarkers at marker component (DonutMarker etc.). I could not find a way to do this at BoundsDriftMarker component.
I know that Dave is working for refactoring SAM, but I didn't check the code so honestly I have no idea how it would affect current logic used in this PR. We will see 😅
I've done some more thinking and understanding your code.
I see why you pass the selectedMarkers
and setSelectedMarkers
through DonutMarker
and ChartMarker
etc but there is a way to do everything in BoundsDriftMarker
...
I just tried this (thanks ChatGPT) in BoundsDriftMarker
and as expected it turns everything yellow.
if (icon != null) icon.options.className += ' highlight-donutmarker';
So we could move all the CSS class logic into BoundsDriftMarker
, and we can add the selectedMarkers
and setSelectedMarkers
props in MapVEuMap.tsx
const finalMarkers = useMemo(() => {
return markers.map((marker) => cloneElement(marker,
{ showPopup: true,
selectedMarkers,
setSelectedMarkers,
}));
}, [markers]);
That should make it possible to leave the old stories unchanged, I think? It should make integration with SAM much simpler too.
It would also be great to get rid of markerType
and markerData
as previously discussed.
It might also be possible to avoid having the consumer component track prevGeohashLevel
?
Maybe MapVEuMap
can prune selectedMarkers
based on the IDs in markers
. So if a selectedMarkers
ID is not present in markers.*.props.id
then remove it from selectedMarkers
. I think that would take care of markers panning off screen too. It might no longer be necessary to handle that using MapVEuMapEvents
. But I haven't thought it all the way through!
@bobular Thank you for your tests 👍 Honestly, it needs to test your suggestions, but I am worried that it may not work for addressing the panning behavior. Initially, I tried to use BoundsDriftMarker component only to handle the re-addition of highlight Class (for panning behavior as I mentioned as b) in my previous comment), but failed: I presumed that it was because of component's update timing, which is often tricky involving multiple components (e.g., among DonutMarker, BoundsDriftMarkers, and MapVEuMap). That was the reason why I moved the logic (re-addition of the Class) to a parent level, i.e., marker component like DonutMarker, etc. because that is the place where marker-related DOM (divs) is re-generated when new data request is made. Though, I might have missed something in my preliminary works.
As I mentioned in the meeting, I would not be able to work for this for about 1.5 weeks at least due to vacations etc., so you may test your logic, especially for map panning, if you are available 😄 🤞
Finally, actually it was quite simpler than I expected to use current PR with SAM. Please see my another draft PR for that, https://github.com/VEuPathDB/web-monorepo/pull/534
@bobular
I have added this multiple marker selection to bubble marker as well. Since there is no story to have bubble marker with data, I utilized SAM for tests: the PR, https://github.com/VEuPathDB/web-monorepo/pull/534, which is associated with SAM, now supports bubble marker as well. You can test multiple marker selection with SAM in the PR. A screenshot is attached in the end of this comment.
As for handling highlighting at BoundsDriftMarker component, not marker components, unfortunatley I could not find a way to do that for panning behavior. As I mentioned earlier, it is probably because of component's update timing for marker data, which is often tricky involving multiple components (e.g., among DonutMarker, BoundsDriftMarkers, and MapVEuMap). That was the reason why I used the logic (re-addition of the Class) to a parent level, i.e., marker component like DonutMarker, etc. because that is the place where marker-related DOM (divs) is re-generated when new data request is made.
@bobular I tried to merge the latest mega change, map refactor by Dave, into this PR. As expected, there were several merge conflicts, which could be resolved, and this multiple marker selection did not work properly. Tried several ways to work this out, however, I could not figure out the reason why the multiple marker selection did not work - couldn't make story file work yet.
Thus, I tried to work for the SAM test PR (https://github.com/VEuPathDB/web-monorepo/pull/534) by re-implemented multiple marker selection to SAM, and found that it worked just fine with little changes on the previous multiple marker selection. Perhaps the way marker data is called in the story has some conflicts in the multiple marker selection under the new map refactor.
For this reason, https://github.com/VEuPathDB/web-monorepo/pull/534 is instead recommended, which also involves the same works implemented in this PR, not this PR, if the multiple marker selection is required.
Note: this PR can be closed as https://github.com/VEuPathDB/web-monorepo/pull/534 was branched out from this PR, has more updates, and is working properly with SAM.
I'm closing this, based on @moontrip's previous comment.
This will address https://github.com/VEuPathDB/web-monorepo/issues/431.
Requirements are:
Notes:
This PR works for both Donut and Chart markers via Marker Selection story.
The selectedMarkers array can be checked via browser's devtool > console.
Most of requirements are done except the last one, panning behavior, which seems to be tricky one: current PR resets selected markers when panning, like zooming or map click. But I will examine it.
Currently, the selectedMarkers array consists of ids as requested. I may be wrong but the purpose of this multi-selection is probably to use selected markers' data for, e.g., a (sub)plot, stats, etc. If those are the cases, then in my opinion, it is better for the selectedMarkers array to contain object(s) (array of objects) that also include(s) data (e.g., species and their counts per marker) etc. instead of (or in conjunction with) id(s). Then it would be much convenient to deal with those selected markers for other purposes. What do you think, @bobular?
some screenshots