ERA-11239: track styles: patrol tracks #1244

luixlive · 2025-02-28T19:25:16Z

What does this PR do?

Re-do the PatrolTrackLegend component so it has the look and feel like the new SubjectTrackLegend.

How does it look

Desktop:

Mobile:

Relevant link(s)

Where / how to start reviewing (optional)

1- Start by reviewing the new component src/TrackLegend/index.js. It's code is around 80% of what SubjectTrackLegend used to be. I just removed the subject internal details so new abstraction is not tied to subjects, and we can also use it to render patrol tracks.
2- After understanding the code of TrackLegend, reviewing PatrolTrackLegend and SubjectTrackLegend will be trivial.
3- The "last" important thing to review is the refactoring and optimization of patrols related selectors in src/selectors/patrols.js.
4- The rest of the files are translations and selector naming updates.

…SubjectTrackLegend and PatrolTrackLegend

…ToPatrolModal

luixlive · 2025-02-28T19:53:45Z

src/Map/index.js

@@ -369,12 +369,6 @@ const Map = ({
    );
  }, [dispatch]);

-  const onPatrolTrackLegendClose = useCallback(() => {


This dispatch is now handled directly by PatrolTrackLegend.

luixlive · 2025-02-28T19:55:50Z

src/PatrolTrackLegend/index.js

@@ -1,110 +1,65 @@
-import React, { memo, useCallback } from 'react';


Implementation of the new TrackLegend component for patrols:

luixlive · 2025-02-28T19:57:09Z

src/PatrolTrackLegend/index.js

-      patrolData={patrolData}
-    />}
-  /> : null;
+  const patrolTrackState = useSelector((state) => state.view.patrolTrackState);


Extremely simple component, we just need to select the patrolTrackState and patrolsWithTrackData values from Redux to calculate the description and the items array (patrols being tracked).

luixlive · 2025-02-28T19:58:58Z

src/SubjectTrackLegend/index.js

@@ -1,125 +1,65 @@
-import React, { useCallback, useEffect, useMemo, useState } from 'react';


There isn't a noticeable change here, but instead of implementing the legend directly here, now we implement the new TrackLegend component passing down the subjects:

luixlive · 2025-02-28T20:00:30Z

src/SubjectTrackLegend/index.js

  const dispatch = useDispatch();
  const { t } = useTranslation('tracks', { keyPrefix: 'subjectTrackLegend' });

-  const timeOfDayTrackingEnabled = useFeatureFlag(FEATURE_FLAG_LABELS.TIME_OF_DAY_TRACKING);
-
-  const isTimeOfDayColoringActive = useSelector((state) => state.view.trackSettings.isTimeOfDayColoringActive);
  const subjectStore = useSelector((state) => state.data.subjectStore);


The implementation is very similar to the PatrolTrackLegend. Here we select subjectStore, subjectTrackState, subjectTracksTrimmedToTrackTimeEnvelope and trackTimeEnvelope to build the description and items (tracked subjects).

luixlive · 2025-02-28T20:02:14Z

src/TrackLegend/TracksList/index.js

@@ -0,0 +1,57 @@
+import React from 'react';


This is not really a new component, it's just the abstraction of what used to be SubjectTrackList so now it's agnostic of the items that are listed. They could either be subjects or patrols.

luixlive · 2025-02-28T20:05:07Z

src/TrackLegend/index.js

@@ -0,0 +1,204 @@
+import React, { useCallback, useEffect, useState } from 'react';


This is the new abstraction that renders track legends. It needs to be implemented and provided with the items, description, callbacks and the flags to render or not certain buttons. SubjectTrackLegend and PatrolTrackLegend use this component to render their legends, they provide subjects and patrols as items respectevely.

The code is very similar to what used to be SubjectTrackLegend, but I removed all the logic related to subjects so now the "items" are injected as dependencies (props) and the component stays agnostic.

luixlive · 2025-02-28T20:08:54Z

src/ducks/patrols.js

-  results: [],
-};
-
-const patrolsReducer = (state = INITIAL_PATROLS_STATE, action) => {


This reducer wasn't needed anymore. We can rely entirely on the patrolStore. And honestly, it was old nasty code, we were basically storing an Axios response object in our store and barely using it (nothing we couldn't achieve using the patrolStore directly).

luixlive · 2025-02-28T20:09:45Z

src/selectors/patrols.js

@@ -1,155 +1,143 @@
-import uniq from 'lodash/uniq';


Total refactor and optimization of our patrol selectors.

luixlive · 2025-02-28T20:10:07Z

src/utils/patrols.js

@@ -228,15 +228,6 @@ export const actualEndTimeForPatrol = (patrol) => {
    : null;
 };

-export const getLeaderForPatrol = (patrol, subjectStore) => {


Unused code.

luixlive · 2025-02-28T20:11:18Z

src/selectors/tracks/index.js

@@ -64,68 +62,45 @@ export const selectHeatmapSubjectTracksTrimmedToTrackTimeEnvelope = createSelect
  )
 );

-const selectPatrolTracksLeaderIds = createSelector(


This selector is no longer needed since the logic to show / hide the subjects tracks when a patrol is being tracked and the subject is the leader of the patrol was a little bit cleaned (lots could be done to improve this flow still, it's buggy, but this is a start).

gaboDev

Just left a couple of minor comments, the only reason I'm not approving is that I don't see test coverage for TrackLegend implementation in PatrolTrackLegend and SubjectTrackLegend is there a reason for that?

gaboDev · 2025-03-04T21:15:53Z

src/TrackLegend/index.js

+    } else {
+      // If there is a menu expanded, we first collapse it and then expand the new one.
+      onCollapseMenu();
+      setTimeout(() => setExpandedMenu(menu), BOOTSTRAP_DEFAULTS.COLLAPSE_TRANSITION_TIME);


Would be worth it to clear this timeout on unmount?

Actually, it's impossible to unmount the component without first finishing the timeout. If you go to the last few lines of this component, it is wrapped by DelayedUnmount by the same amount of time of this timeout: BOOTSTRAP_DEFAULTS.COLLAPSE_TRANSITION_TIME. This means that even if the component is about to be unmounted, it will at least wait for this time to elapse, making the cleaning task unnecessary 😄

gaboDev · 2025-03-04T21:16:31Z

src/TrackLegend/index.js

+    onExpandMenu(MENUS.TIME_OF_DAY_SETTINGS);
+  };
+
+  const onDectivateTimeOfDayColoring = useCallback(() => {


little type in here, it should be onDeactivateTimeOfDayColoring

gaboDev · 2025-03-04T21:19:02Z

src/TrackLegend/index.js

+    dispatch(setIsTimeOfDayColoringActive(false));
+
+    // When deactivating the time of day coloring, we collapse its menu if it was expanded.
+    if (expandedMenu === MENUS.TIME_OF_DAY_SETTINGS) {


Several ifs statements are repeating the same comparisons of the const at the beginning of the component isTimeOfDaySettingsExpanded isTrackSettingsExpanded isTracksListExpanded Why are we using those here?

Duh, didn't think of it 🤓 Updating them!

gaboDev · 2025-03-04T21:25:28Z

src/SubjectTrackLegend/index.js

-  };
+  // Build the items array with the description, icon, id and title of each tracked subject.
+  const items = useMemo(() => subjectTracksTrimmedToTrackTimeEnvelope.map((subjectTracks) => {
+    const id = subjectTracks.track.features[0].properties.id;


Here seems like a good place to use:
const [firstFeature] = subjectTracks.track.features;
instead of repeating subjectTracks.track.features[0] just to make the code more readable

Also, is it possible to subjectTracks.track.features[0] to does not exist? If yes, We should add some default values or validations

On the first comment: agreed, I'll update it.
On the second one: it shouldn't, that data comes from a selector that is already running validations and filtering just the valid data. If we ever hit that issue, I wouldn't say the solution is adding a check here but fixing the selector.

luixlive · 2025-03-04T21:48:24Z

Just left a couple of minor comments, the only reason I'm not approving is that I don't see test coverage for TrackLegend implementation in PatrolTrackLegend and SubjectTrackLegend is there a reason for that?

@gaboDev Nice observation! Actually I forgot to add a comment regarding this myself. I decided not to add tests to those. I tried, but the necessary mocked data in the store for the selectors to build the right scenarios is so complex and big, that the tests would be extremely fragile and hard to comprehend. I know this is not orthodox, we should try to add tests anyway, but for real, the amount of data was insane, and in every scenario it changes considerably. In my opinion, components shouldn't rely on these super complex data selection. Our selectors are doing too much. I would say that the right fix is to update our reducers to actually do some pre-processing before storing the data and avoid having to do all that processing in our selectors. That would optimize time and memory, while also simplifying the data mocking in the tests. But that was definitely out of the scope. This PR was big enough.

ERA-11239: Abstract the TrackLegend component so it can be reused by …

2f677e2

…SubjectTrackLegend and PatrolTrackLegend

luixlive added the deploy label Feb 28, 2025

luixlive marked this pull request as draft February 28, 2025 19:25

luixlive temporarily deployed to ERA-11239 February 28, 2025 19:25 — with GitHub Actions Inactive

luixlive had a problem deploying to padas-app February 28, 2025 19:25 — with GitHub Actions Failure

ERA-11239: Define the initial state for the patrol reducer in the Add…

d371e12

…ToPatrolModal

luixlive temporarily deployed to ERA-11239 February 28, 2025 19:33 — with GitHub Actions Inactive

luixlive temporarily deployed to padas-app February 28, 2025 19:34 — with GitHub Actions Inactive

luixlive commented Feb 28, 2025

View reviewed changes

ERA-11239: Add unit tests

251f4c1

luixlive temporarily deployed to ERA-11239 March 3, 2025 20:21 — with GitHub Actions Inactive

luixlive temporarily deployed to padas-app March 3, 2025 20:21 — with GitHub Actions Inactive

luixlive marked this pull request as ready for review March 3, 2025 20:23

luixlive requested review from gaboDev, JoshuaVulcan and AlanCalvillo March 3, 2025 20:24

gaboDev requested changes Mar 4, 2025

View reviewed changes

luixlive added 2 commits March 4, 2025 15:37

Merge branch 'develop' into ERA-11239

d362e69

ERA-11239: Minor fixes as suggested in the PR

74734de

luixlive temporarily deployed to ERA-11239 March 4, 2025 21:44 — with GitHub Actions Inactive

luixlive temporarily deployed to padas-app March 4, 2025 21:44 — with GitHub Actions Inactive

luixlive requested a review from gaboDev March 4, 2025 21:48

gaboDev approved these changes Mar 4, 2025

View reviewed changes

luixlive merged commit 89873a4 into develop Mar 4, 2025
3 checks passed

luixlive deleted the ERA-11239 branch March 4, 2025 22:23

		@@ -1,110 +1,65 @@
		import React, { memo, useCallback } from 'react';

		@@ -1,125 +1,65 @@
		import React, { useCallback, useEffect, useMemo, useState } from 'react';

		@@ -0,0 +1,204 @@
		import React, { useCallback, useEffect, useState } from 'react';

ERA-11239: track styles: patrol tracks #1244

ERA-11239: track styles: patrol tracks #1244

Uh oh!

Conversation

luixlive commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

How does it look

Relevant link(s)

Where / how to start reviewing (optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luixlive Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaboDev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luixlive commented Mar 4, 2025

Uh oh!

Uh oh!

Uh oh!

luixlive commented Feb 28, 2025 •

edited

Loading

luixlive Feb 28, 2025 •

edited

Loading