SAFB-249: Group Icon Functionality #305

RoryMacGregor88 · 2022-09-29T10:33:47Z

Closes #SAFB-249

This allows group icons to be interacted with the same way as cluster icons and individual icons.

Fixed the pin functionality for the group icons, which turned out to be a pretty tricky change. It involved updating the base-level PinLayer to recognise the difference between a cluster and a group, then call the passed getPinColor method in that case. Then the function in mapHelpers that determines pin color had to be updated to also recognise groups, and work out if the selected item is in that group by analyising the coordinates of each. Finally, each of the Chatbot screens had to be updated to find the selected item from the list of all, then pass the id and coordinates to the layer handler in order to make all of the above possible.

Added the fix to allow group icons to update to the in-situ cameras panel. There were some strange bugs here that greatly slowed down developent, one of which was there being a second GeoJsonPinLayer in the alertList for some reason, that was resetting the icons as soon one was clicked. And the other was the cameraList being initialised to an array, even though the returned data is a featureCollection object.

src/components/BaseMap/PinLayer.js

src/helpers/mapHelper.js

src/pages/Chatbot/Comms/index.js

Reverted removing the duplicated GeoJsonPinLayer in in-situ, as it doesn't need to convert to GeoJson like the helper one does. However, I had to extend the dupliated one to 1. include the pinInfo required to identify the selected icon, and 2. pass this function through to the list so that both the list and map are sharing the same GeoJsonLayer.

After some refactoring, the clicks for in-situ cameras (which work a bit differently from the others) are working as expected. This involved a bit of juggling between when to use alert ids and camera ids, but the camera ids seem to be the correct choice for this kind of identification. Also refactored the PinLayer to be able to differentiate between clusters and groups in the getIconColor function, and the generic mapHelper to be able to tell if incoming data is already a feature collection before reshaping.

Refactores the click handler for in-situ to make it cleaner, and fixed the events group click as it was not working correctly.

Reverted the condition added to the PinLayer, as it turns out it is unnecessary. IssueID: SAFB-249

By searching for the selected icon inside an array of cluster leaf node ids, it allows for pinpoint accurate selection, as opposed to the coordinate match method used before, which worked (kind of), but was not nearly as accurate. Is also much simpler code to read.

Just moved a color from being hardcoded in place to using a constant.

allynt · 2022-10-11T09:32:09Z

Hmm... I've just noticed that not all the clustered pins have the correct circle:

marksmall

Few changes requested, nothing that major though.

marksmall · 2023-01-17T15:31:53Z

src/helpers/mapHelper.js

+ properties,
+ geometry,
+ };
+})


I've seen this self same code in stuff @stevefaeembra has worked on, enough times that maybe it should be a utility function. Just a suggestion, for now.

marksmall · 2023-01-17T15:33:27Z

src/pages/Chatbot/Comms/index.js

@@ -70,7 +70,14 @@ const Comms = () => {

 useEffect(() => {
 if (allReports.length > 0) {
- setIconLayer(getIconLayer(allReports, MAP_TYPES.COMMUNICATIONS, 'communications', dispatch, setViewState, {id: commID}));
+


Empty line as the first bit of the if isn't great, in my opinion, but it is just my opinion, I know some devs like it and others seem to accidentally do it.

marksmall · 2023-01-17T15:33:52Z

src/pages/Chatbot/Missions/index.js

@@ -84,7 +84,14 @@ const Missions = () => {

 useEffect(() => {
 if (allMissions.length > 0) {
- setIconLayer(getIconLayer(allMissions, MAP_TYPES.MISSIONS, 'target', dispatch, setViewState, {id: missionId}));
+


marksmall · 2023-01-17T15:35:43Z

src/pages/Chatbot/Missions/index.js

+ ? { center: selectedMission.location, id: missionId }
+ : {}
+
+ setIconLayer(getIconLayer(allMissions, MAP_TYPES.MISSIONS, 'target', dispatch, setViewState, pinInfo));


The code in these 2 useEffects are almost identical, find an item by it's id in an array, then create an object from it and other info, then create an icon layer. I get that the arrays are differnt and the Map_TYPES etc, but the idea is the same, that is ripe for a utility function. Repeating it twice isn't a big deal, but what if you had to do it 10 times, for instance.

marksmall · 2023-01-17T15:36:37Z

src/pages/Chatbot/People/index.js

@@ -78,7 +78,14 @@ const People = () => {

 useEffect(() => {
 if (allPeople.length > 0) {
- setIconLayer(getIconLayer(allPeople, MAP_TYPES.PEOPLE, 'people', dispatch, setViewState, {id: peopleId}));
+


and again, leading empty line.

marksmall · 2023-01-17T15:37:03Z

src/pages/Chatbot/People/index.js

+ ? { center: selectedPeople.location, id: peopleId } 
+ : {};
+
+ setIconLayer(getIconLayer(allPeople, MAP_TYPES.PEOPLE, 'people', dispatch, setViewState, pinInfo));


Same comment about a utility function here as I made before.

marksmall · 2023-01-17T15:37:12Z

src/pages/Chatbot/Reports/index.js

+ ? { center: selectedReport.location, id: reportId } 
+ : {};
+
+ setIconLayer(getIconLayer(reshapedReports, MAP_TYPES.REPORTS, 'report', dispatch, setViewState, pinInfo, 'report_id'));


marksmall · 2023-01-17T15:40:09Z

src/pages/In-situ/Components/Alert.js

@@ -77,7 +77,7 @@ const Alert = ({ card, alertId, setSelectedAlert, setFavorite, t }) => {
 return (
 <>
 <Card
- onClick={() => setSelectedAlert(card.id)}
+ onClick={() => setSelectedAlert(card.id, card.camera_id)}


This is a judgement call really, passing in potentially many params, rather than grouping them and passing a single object and leaving it up to the setSelectedAlert function to figure out what it needs from the card object. Say in future, you had to add another parameter, you keep changing the interface of the function, so other uses might need to pass in null|undefined, whereas if it just took an object, the interface doesn't change, but as I say, at this point, it is a judgement call

marksmall · 2023-01-17T15:41:08Z

src/pages/In-situ/Components/AlertList.js

+ setCameraId(cameraId)
+ }
+ }
+
 return (
 <>


Not your code I get it, but is this fragment <></> needed, does the Row not suffice?

marksmall · 2023-01-17T15:42:00Z

src/pages/In-situ/index.js

+ : {};
+
+ setIconLayer(getIconLayer(cameraList.features, MAP_TYPES.IN_SITU, 'camera', dispatch, setViewState, pinInfo));
+ }


Same comment as before about the utility function. That is 4 occurrences of the same idea, just using different params.

RoryMacGregor88 added 2 commits September 27, 2022 16:31

RoryMacGregor88 requested review from allynt and stevefaeembra September 29, 2022 10:33

allynt reviewed Sep 29, 2022

View reviewed changes

src/components/BaseMap/PinLayer.js Outdated Show resolved Hide resolved

allynt reviewed Sep 29, 2022

View reviewed changes

src/helpers/mapHelper.js Show resolved Hide resolved

allynt reviewed Sep 29, 2022

View reviewed changes

src/pages/Chatbot/Comms/index.js Show resolved Hide resolved

RoryMacGregor88 added 6 commits September 29, 2022 12:58

fix(frontend): Refactor in-situ clicks, fix events group click

1e5ea3e

Refactores the click handler for in-situ to make it cleaner, and fixed the events group click as it was not working correctly.

fix(frontend): Revert changes to PinLayer

802d095

Reverted the condition added to the PinLayer, as it turns out it is unnecessary. IssueID: SAFB-249

fix(frontend): Move color to constant

ac887cf

Just moved a color from being hardcoded in place to using a constant.

marksmall requested changes Jan 17, 2023

View reviewed changes

marksmall force-pushed the development branch from 99bb90e to fe3b09a Compare February 1, 2023 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAFB-249: Group Icon Functionality #305

SAFB-249: Group Icon Functionality #305

RoryMacGregor88 commented Sep 29, 2022 •

edited by allynt

Loading

allynt commented Oct 11, 2022

marksmall left a comment

marksmall Jan 17, 2023

marksmall Jan 17, 2023

marksmall Jan 17, 2023

marksmall Jan 17, 2023

marksmall Jan 17, 2023

marksmall Jan 17, 2023

marksmall Jan 17, 2023

marksmall Jan 17, 2023

marksmall Jan 17, 2023

marksmall Jan 17, 2023

SAFB-249: Group Icon Functionality #305

Are you sure you want to change the base?

SAFB-249: Group Icon Functionality #305

Conversation

RoryMacGregor88 commented Sep 29, 2022 • edited by allynt Loading

allynt commented Oct 11, 2022

marksmall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoryMacGregor88 commented Sep 29, 2022 •

edited by allynt

Loading