EDA Map Refactor: Introduce MapType plugin and implementations #424

dmfalke · 2023-08-15T17:22:48Z

This PR introduces the MapType plugin concept, and implementations for donut, bar, and bubble map types.

Some follow-up tasks that we might consider:

Additional cleanup and DRY up
This should be an issue... Change to "fly to" behavior. Currently it is only inspecting zoom level, but not center. That might be good enough. The rough idea for a bigger change is to change how this is triggered (not based on zoom level, but as callback that can be called by click handlers, etc), and to change the behavior such that the "global" markers are loaded in the background, the bounds are determined, and then the map is zoomed to those bounds.

TODO:

Extract maker logic from MapVEuMap component
Implement MapType plugins
Address code duplication <-- This is partially done, but more can easily be done in the future.
Add MapTypeHeaderDetails component for rendering counts
Reapply flyToMakers logic in each MapType

bobular

This looks great. The diff's a bit confusing but I browsed the code and the relocation of the marker logic makes perfect sense. Is there anything specifically "dodgy" you want feedback on?

The stories all work fine - animations, flyTo etc.

packages/libs/components/src/map/SemanticMarkers.tsx

dmfalke · 2023-08-18T18:45:39Z

If the animations look good, I'm happy. I cleaned up some useEffect hooks in SemanticMarkers, and ran into some animation issues with zooming in, at first. They should all be resolved now.

I plan to continue working on this branch. Next, and hopefully last, up is implementing the new MapTypePlugins.

…ariable-map-type--map-type-refactor

…' into map-collection-variable-map-type--map-type-refactor

- Remove `getData` from `MapTypePlugin` interface - Utilize react-query to cache network requests - Move constants from index module, to prevent circular imports

…maker data

…ariable-map-type--map-type-refactor

bobular

Should constrainLongitudeToMainWorld in SemanticMarkers be called from somewhere?
Ah... I see it has moved to MapVEuMap.tsx https://github.com/VEuPathDB/web-monorepo/pull/424/files#diff-67456b6e516de477c34a0a21ba3c750d25463bbc48e5059a0fe59bb8c44941efR465
So it looks like the SemanticMarkers version can be removed.

That's my review of the components part done. It's code-approved but pending testing. I'm going to submit this as a comment and review the eda side after lunch! I recently lost a big github issue draft so don't want the same to happen to my review.

packages/libs/components/src/map/SemanticMarkers.tsx

dmfalke · 2023-10-23T15:11:53Z

Thanks for the review, @bobular. I will remove those out-of-date things.

bobular

Been through all the eda code and approve in principal but would like to test tomorrow ideally. Especiallly figure out the flyTo stuff.

bobular · 2023-10-23T13:51:31Z

packages/libs/eda/src/lib/map/MapVeuContainer.tsx

+          )}
+        />
+      </Switch>
+      <ReactQueryDevtools initialIsOpen={false} />


Does this stay in for ever? Was it true a week ago when I last tested?

Good question. This has been false since I added it. The docs say that this will not appear in prod builds (>= qa). It can be useful, but the icon could get in the way, I suppose.

Related: it might make sense to move react-query to wdk-client or web-common.

Yeah, I was wondering about the flower icon. Fingers crossed it goes away in prod - otherwise an easy fix!

We'll know pretty quickly, since qa sites use a "prod" build

packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx

packages/libs/eda/src/lib/map/analysis/MapHeader.tsx

packages/libs/eda/src/lib/map/analysis/MarkerConfiguration/PieMarkerConfigurationMenu.tsx

packages/libs/eda/src/lib/map/analysis/mapTypes/plugins/BarMarkerMapType.tsx

packages/libs/eda/src/lib/map/analysis/mapTypes/plugins/BubbleMarkerMapType.tsx

packages/libs/eda/src/lib/map/analysis/mapTypes/types.ts

bobular · 2023-10-24T11:27:49Z

Hi @dmfalke - I did some testing and thought we had to bring back the old (but flawed) flyto trigger until we fix it in the way you suggest in this PR's description. The problem was that the user might want to zoom out to zoom level 1 without it automatically zooming back in again.

See my suggestion here --> #566

…tion Bring back old FlyTo behaviour for now

bobular

I tested today and I'm sure there will be some rough edges to smooth (including handling of empty results which I am on the case for) but it's working great and I suggest we go for merge!

bobular · 2023-10-24T21:47:41Z

I think the header's Add filters button might be broken.

* Restructure side menu to allow for subheadings. (#398) Changes include: - Variable renaming - Modifications to finding active menu item and active map type - Extracting some components * Some additional clean-up (#401) * Move active viz to realm of map type config, and clean up some labels and ids. * Have the active map type config panel open by default * Remember whether side panel is expanded on subsequent page loads * Remove unused app state property * EDA Map Refactor: Introduce MapType plugin and implementations (#424) * Remove marker-related props from MapVEuMap * Move onBoundsChange logic to MapVEuMap; clean up SemanticMarkers hooks * Extract getDefaultAxisRange function * Move DraggableLegendPanel to its own module * Move defaultAnimation to its own module * Extract bubble overlay config into its own function * Add MapType plugins * Wire up MapType plugins * Use plugin displayName in menu * Reduce props passed to getData * Checkpoint commit using @tanstack/query * Incorporate additional changes from merge * Checkpoint - Remove `getData` from `MapTypePlugin` interface - Utilize react-query to cache network requests - Move constants from index module, to prevent circular imports * Add TODO * Propagate selected values for marker configuration to overlay config * Remove TODO * Keep previous query data; use isFetching status to show spinners for maker data * Introduce `MapTypeHeaderDetails` to `MapTypePlugin` * Remove unused type * Crudely reimplment "flyToData" * Pass valueSpec to standalone map markers * Remove out of date comments and code * Remove old comments * Move shared interface to shared file * Use destructured variables * bring back old behaviour for now --------- Co-authored-by: Bob <uncoolbob@gmail.com> * Post-merge updates * Post-merge updates, 2 * Fix filter button in header * remove file --------- Co-authored-by: Connor Howington <connor.howington@gmail.com> Co-authored-by: Bob <uncoolbob@gmail.com>

Remove marker-related props from MapVEuMap

98f56fb

dmfalke changed the base branch from main to map-collection-variable-map-type August 15, 2023 17:23

dmfalke added 2 commits August 17, 2023 14:13

Move onBoundsChange logic to MapVEuMap; clean up SemanticMarkers hooks

f9e464e

Clean up imports

b2422a0

bobular reviewed Aug 18, 2023

View reviewed changes

packages/libs/components/src/map/SemanticMarkers.tsx Show resolved Hide resolved

dmfalke added 9 commits August 18, 2023 14:54

Merge branch 'map-collection-variable-map-type' into map-collection-v…

51aa36b

…ariable-map-type--map-type-refactor

resolve some merge conflicts

491407a

Extract getDefaultAxisRange function

a2051e0

Move DraggableLegendPanel to its own module

6320cb9

Move defaultAnimation to its own module

0a70540

Extract bubble overlay config into its own function

a633ff9

Add MapType plugins

1712229

Wire up MapType plugins

f148ec5

Merge branch 'map-collection-variable-map-type' into map-collection-v…

64d8a97

…ariable-map-type--map-type-refactor

dmfalke mentioned this pull request Aug 24, 2023

Map type plugin feature #399

Merged

dmfalke added 14 commits August 24, 2023 14:20

Use plugin displayName in menu

5995eb7

Reduce props passed to getData

c3a1ea8

Checkpoint commit using @tanstack/query

c189122

Merge remote-tracking branch 'origin/map-collection-variable-map-type…

4d4022c

…' into map-collection-variable-map-type--map-type-refactor

Incorporate additional changes from merge

8f7e9d0

Checkpoint

62a0832

- Remove `getData` from `MapTypePlugin` interface - Utilize react-query to cache network requests - Move constants from index module, to prevent circular imports

Add TODO

faddbd2

Propagate selected values for marker configuration to overlay config

a7e6413

Remove TODO

e6c6aa0

Keep previous query data; use isFetching status to show spinners for …

73c3518

…maker data

Introduce MapTypeHeaderDetails to MapTypePlugin

a4a7528

Remove unused type

e221583

Crudely reimplment "flyToData"

fab748c

Merge branch 'map-collection-variable-map-type' into map-collection-v…

84550db

…ariable-map-type--map-type-refactor

Pass valueSpec to standalone map markers

b226c3c

dmfalke changed the title ~~WIP: Map collection variable map type map type refactor~~ EDA Map Refactor: Introduce MapType plugin and implementations Oct 19, 2023

dmfalke marked this pull request as ready for review October 19, 2023 14:37

bobular reviewed Oct 23, 2023

View reviewed changes

packages/libs/components/src/map/SemanticMarkers.tsx Outdated Show resolved Hide resolved

Remove out of date comments and code

d50bafd

bobular reviewed Oct 23, 2023

View reviewed changes

dmfalke and others added 4 commits October 23, 2023 12:08

Remove old comments

44b5bbb

Move shared interface to shared file

4b94c95

Use destructured variables

0386c1b

bring back old behaviour for now

2922f41

Merge pull request #566 from VEuPathDB/map-type-refactor-flyto-sugges…

0a3b318

…tion Bring back old FlyTo behaviour for now

bobular self-requested a review October 24, 2023 17:43

bobular approved these changes Oct 24, 2023

View reviewed changes

This was referenced Oct 24, 2023

Map - handle empty map-markers responses elegantly #571

Closed

Map - unwanted 400 error-producing map-markers requests during variable change (post refactor) #572

Closed

dmfalke merged commit f6e39d3 into map-collection-variable-map-type Oct 25, 2023
1 check passed

dmfalke deleted the map-collection-variable-map-type--map-type-refactor branch October 25, 2023 14:22

This was referenced Nov 1, 2023

Multiple marker selection branch out #581

Merged

multi-selecting markers #513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EDA Map Refactor: Introduce MapType plugin and implementations #424

EDA Map Refactor: Introduce MapType plugin and implementations #424

dmfalke commented Aug 15, 2023 •

edited

Loading

bobular left a comment

dmfalke commented Aug 18, 2023

bobular left a comment

dmfalke commented Oct 23, 2023

bobular left a comment

bobular Oct 23, 2023

dmfalke Oct 23, 2023

bobular Oct 24, 2023

dmfalke Oct 24, 2023

bobular commented Oct 24, 2023 •

edited

Loading

bobular left a comment

bobular commented Oct 24, 2023

EDA Map Refactor: Introduce MapType plugin and implementations #424

EDA Map Refactor: Introduce MapType plugin and implementations #424

Conversation

dmfalke commented Aug 15, 2023 • edited Loading

bobular left a comment

Choose a reason for hiding this comment

dmfalke commented Aug 18, 2023

bobular left a comment

Choose a reason for hiding this comment

dmfalke commented Oct 23, 2023

bobular left a comment

Choose a reason for hiding this comment

bobular Oct 23, 2023

Choose a reason for hiding this comment

dmfalke Oct 23, 2023

Choose a reason for hiding this comment

bobular Oct 24, 2023

Choose a reason for hiding this comment

dmfalke Oct 24, 2023

Choose a reason for hiding this comment

bobular commented Oct 24, 2023 • edited Loading

bobular left a comment

Choose a reason for hiding this comment

bobular commented Oct 24, 2023

dmfalke commented Aug 15, 2023 •

edited

Loading

bobular commented Oct 24, 2023 •

edited

Loading