multi-selecting markers

[moontrip]

This will address #431.

Requirements are:

• single click on marker selects
• selected markers are highlighted somehow (legacy map had single-select and used a yellow outline)
• single click on another marker adds to the selection
• single click on selected marker unselects it
• a single click on the map (not a marker) deselects all
• any change in geohash zoom level cancels the selections
• any marker that "falls off" the viewport while panning the map becomes unselected

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
  

[bobular]

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)?

[moontrip]

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.

[moontrip]

Update:

• Refactored codes. Now, selectedMarker have as many information as possible including data.
• The last item in the top description, "any marker that "falls off" the viewport while panning the map becomes unselected" is implemented. When map panning occurs, selectedMarkers are checked with current map bounds and then selected markers that are out-of-bounds markers are excluded and highlighting is also removed. Note that map panning leads to new data request, thus it is inevitable to see on/off highlighting because highlighting is based on adding/removing HTML class (CSS)

To-do:

• Checking Bob's request, "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)?"
• It is necessary to check this PR with the SAM. This will be done by branching out this PR's branch, just in case.

[moontrip]

@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.

[bobular]

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 the highlighted-marker class all by itself, and that BoundsDriftMarker should have two optional callback props onSelected and onUnselected that expect the marker ID as an argument.

These callbacks could be added in MapVEuMap where the management of the selectedMarkers 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 and onSelectedMarkersChange props would belong to SemanticMarkers (and the finalMarkers map bit would go in there too (where it has been renamed refinedMarkers)).

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! ;-)

[bobular]

This isn't going to be very maintainable. We already have to think about bubble markers.

Please instead use a single class highlighted-marker and use CSS rules in vb-popbio-maps.css something like the following:

.highlighted-marker {
  background-clip: padding-box;
  z-index: 9999 !important;

  &.chart-marker {
    /* the rounded corner of the chart marker has 10px in radius */
    border-radius: 10px;
    /* add inset to reduce a gap between the element and the box-shadow */
    box-shadow: -1px -0.5px 0px 6px rgba(255, 255, 0, 1),
      -1px -0.5px 0px 2px rgba(255, 255, 0, 1) inset;
  }

  &.donut-marker { /* probably add `&.bubble-marker` here too, after adding it in BubbleMarker.tsx's `L.divIcon()` */
  border-radius: 50%;
    /* add inset to reduce a gap between the element and the box-shadow */
    box-shadow: 2.5px 2.5px 0px 6px rgba(255, 255, 0, 1),
      2.5px 2.5px 0px 2px rgba(255, 255, 0, 1) inset;
  }
}

[moontrip]

@bobular Yes it could have been used if it belonged to our case but the "&" approach is for SaSS/SCSS grammar, not for pure CSS. Perhaps I can use it at separate SCSS file and load it accordingly. Thank you for suggesting this 👍 and I will check it.

[bobular]

Oh, didn't know that! Then just do

.highlighted-marker {
  ...
}
.highlighted-marker.donut-marker {
   ...
}
.highlighted-marker.chart-marker {
  ...
}

[moontrip]

Hi @bobular 👋

I have made some updates on this PR after checking/testing your suggestions. Will leave my answer to the corresponding one.

For this case, it turned out that your approach could not be achieved. It is because there is no common CSS that affects highlight: those background-clip and z-index didn't have little impact. Thus, unfortunately we need to keep the current format.

[bobular]

Sorry I didn't answer your question about adding extra data to the selectedMarkers.

What's the benefit of all this extra data to the consumer component (e.g. MapAnalysis for the SAM)

Why are we already committing to only donut and chart here? (not bubble)

[moontrip]

@bobular So, your question is related to why we need to select markers and I think you envisioned it? 😄 I thought that some use cases were: a) plot only for selected markers; b) compute stats from the selected markers. Assuming that those are correct, I thought that we would not need to request data to the backend again if we have embedded data for the selected markers. I might be wrong 😞

As for your last question, yes I didn't think of the bubble marker case as I just thought you intended for donut and chart markers only. I will see if it is doable for bubble marker.

[bobular]

Thanks for your use case suggestions.

For a) if you mean "supporting plots", the geohash IDs will be sufficient to add as "little filters" (e.g. `geohash_3 IN ('abc', 'def', ghi')) for those.

For b) I did think that maybe the per-marker entityCount might be useful for a fourth row in the map header's summary counts, but that's not currently in your markerDataProp type. It's not worth thinking about now - things will change a bit with Dave's refactor (each map mode will probably "know how to" generate the header information - and we can easily calculate the "fourth number" by filtering the original marker data using the IDs).

Tracking just the geohash IDs will not require further requests - don't worry.

[moontrip]

@bobular Thank you for sharing your thoughts. I have commented out the data prop for the markerData

[bobular]

Why are we passing props.selectedMarkers and props.setSelectedMarkers into chartMarkerSVGIcon?

All it does is return them.

[moontrip]

@bobular Good question! I will check 👍

[moontrip]

@bobular I have removed unnecessary parts and simplified them

[moontrip]

Hi @bobular 👋 Thank you for your review and sharing your thoughts 👍 I will reply them one-by-one later.

[moontrip]

@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 the highlighted-marker class all by itself, and that BoundsDriftMarker should have two optional callback props onSelected and onUnselected that expect the marker ID as an argument.

These callbacks could be added in MapVEuMap where the management of the selectedMarkers 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 and onSelectedMarkersChange props would belong to SemanticMarkers (and the finalMarkers map bit would go in there too (where it has been renamed refinedMarkers)).

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, 36e3809, 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 😅

[bobular]

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.

[bobular]

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!

[moontrip]

@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, #534

[moontrip]

@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, #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.

[moontrip]

@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 (#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, #534 is instead recommended, which also involves the same works implemented in this PR, not this PR, if the multiple marker selection is required.

[moontrip]

Note: this PR can be closed as #534 was branched out from this PR, has more updates, and is working properly with SAM.

[dmfalke]

I'm closing this, based on @moontrip's previous comment.