ERA-11057: track styles: track color expression #1243

gaboDev · 2025-02-27T22:19:02Z

What does this PR do?

Adds time-of-day coloring expression to track points
Adds batch support to useMapSource and useMapLayer hooks

How does it look

Relevant link(s)

Tracking tickets: ERA-11057
Hosted demo environments

…ux coloring flag

…led prop from source

gaboDev · 2025-03-03T15:51:01Z

src/selectors/tracks/index.js

+      return isTimeOfDayColoringActive
+        ? {
+          ...trimmedTrackData,
+          twoPointLineStringTrackPoints: buildFeatureCollectionOfTwoPointLineStringSegments(trimmedTrackData.track, timeOfDayTimeZone)


We need to split all track point by segments formed by a line string object of 2 points having: the coors and times of point A and B, also assigning a startColor and endColor based on the time of each point

This object will allow us to set an specific gradient color to each line, being a workaround of the issue of MapBox not being able to apply dynamically data-drive stop colors for a gradient line

trackSegments would suffice as a name here. that's really all it is.

gaboDev · 2025-03-03T15:54:53Z

src/TracksLayer/track.js

+    sourcesConfigs,
+    layersConfigs,
+    hasTimeOfDaySegments
+  } = useMemo(() => generateMapSourcesAndLayersBasedOnTwoLineTrackPointsSegments(


Here, two things are happening internally:

We grab the two-line segments calculated on the selector and create groups of the all possible combination of pairs of color stops (64), this with the purposes of reduce the amount of sources/layers created

For each pair, We create layer/source config objects to be passed to the map

gaboDev · 2025-03-03T15:59:37Z

Most of the changes of this PR are related with the refactor of useMapSource and useMapLayer, since the approach for the coloring expression required some grouping it was necessary to adapt those hooks to be use with batches of object configs.

The actual approach can be seen in these files:

src/selectors/tracks/index.js
src/TracksLayer/track.js
src/TracksLayer/utils/index.js

luixlive

Just a first pass. The intention is clear. I haven't tested the code yet, we would need some nice data in the environment. Also I'll wait for the missing tests you mentioned. But for now I have several small suggestions to try to make this piece of code as readable as possible since it's a very specific feature.

src/hooks/index.js

luixlive · 2025-03-03T15:30:50Z

src/hooks/useMapLayer/index.js

+              id,
+              source: sourceId,
+              type,
+              layout: layout || {},


You are already defining layout as layout = {} so adding a default here again is redundant.

src/hooks/useMapLayer/index.js

src/hooks/useMapSource/index.js

src/TracksLayer/track.js

src/TracksLayer/utils/index.js

luixlive · 2025-03-03T17:51:47Z

src/selectors/tracks/index.js

+      return isTimeOfDayColoringActive
+        ? {
+          ...trimmedTrackData,
+          twoPointLineStringTrackPoints: buildFeatureCollectionOfTwoPointLineStringSegments(trimmedTrackData.track, timeOfDayTimeZone)


I understand the reasoning behind this naming twoPointLineStringTrackPoints (for both the property and the method) right now, but it worries me that it will not be intuitive when we come back two months later and read this code. It feels a bit obscure. Could you think of a name a bit more explicit about its relation with the time of day feature? buildTimeOfDayFeatureCollection maybe. And of course, add comments inside the method explaining that the feature collection consists of several line strings connecting each two coordinates.

@luixlive @gaboDev I would more recommend a very accurate-and-generic name. There's nothing time-of-day specific here, and as we increase style customization it might be pretty desirable and recyclable to have these segmented tracks. I like something simple and clean like buildTrackSegments(track) IMHO.

Well, it sounds like we want to define "segment" like a feature collection of line strings joining pairs of points. I'm ok with that, it would be nice to document it somewhere, even just as a comment in the method so its clear what we mean with that.

But I differ about something else. You say @JoshuaVulcan that there is nothing related to the time of day feature, but there is: the start color and end color properties added to these "segments":

That is part of the method and it is specifically related to that feature. That's why I was thinking on a name that links this method with the feature. If we want to put a generic name to it I would recommend to not add those properties there, add them separately. But I don't know if it's really worth it and if we are going to reuse this method later. It sounds like we can leave it as is for now and then refactor if it's necessary.

Sure, I agree with trackSegments for the prop and buildTrackSegments for the method

Just consider the fact that it has logic related to the time of day period feature. trackSegments sounds very generic and doesn't tell what is really going on. We could remove that logic from there to keep it generic, or make sure that the name expresses the intent.

JoshuaVulcan

Pretty light-touch first pass, mostly concerned with supplementing existing comments and looking at code style/naming conventions. It's close!

JoshuaVulcan · 2025-03-03T19:28:11Z

src/hooks/useMapSource/index.js

+  const map = useContext(MapContext);
+  const sourceIdsRef = useRef([]);
+  const sourcesConfigs = useMemo(() => Array.isArray(sourceConfig) ? sourceConfig : [sourceConfig], [sourceConfig]);
+


Why not just have a plural name (such as useMapSources) and have it always accept an array of objects? That would be clearer and simpler than accepting these two formats for sourceConfig, and take the mystery/magic out of naming it useMapSource but secretly allowing more than one source at a time in the hook

JoshuaVulcan · 2025-03-03T19:29:49Z

src/hooks/useMapSource/index.test.js

+    jest.useRealTimers();
+  });
+
+});


Missing tests for multi-source scenarios

JoshuaVulcan · 2025-03-03T19:30:08Z

src/selectors/tracks/index.js

-import { getTimeOfDayPeriodBasedOnTime, trimTrackDataToTimeRange } from '../../utils/tracks';
+import {
+  trimTrackDataToTimeRange,
+  buildFeatureCollectionOfTwoPointLineStringSegments


I think Luis already mentioned, but buildFeatureCollectionOfTwoPointLineStringSegments is a pretty stanky and confusing name

JoshuaVulcan · 2025-03-03T19:31:54Z

src/selectors/tracks/index.js

+      return isTimeOfDayColoringActive
+        ? {
+          ...trimmedTrackData,
+          twoPointLineStringTrackPoints: buildFeatureCollectionOfTwoPointLineStringSegments(trimmedTrackData.track, timeOfDayTimeZone)


trackSegments would suffice as a name here. that's really all it is.

JoshuaVulcan · 2025-03-03T19:33:46Z

src/selectors/tracks/index.js

+      return isTimeOfDayColoringActive
+        ? {
+          ...trimmedTrackData,
+          twoPointLineStringTrackPoints: buildFeatureCollectionOfTwoPointLineStringSegments(trimmedTrackData.track, timeOfDayTimeZone)


@luixlive @gaboDev I would more recommend a very accurate-and-generic name. There's nothing time-of-day specific here, and as we increase style customization it might be pretty desirable and recyclable to have these segmented tracks. I like something simple and clean like buildTrackSegments(track) IMHO.

JoshuaVulcan · 2025-03-03T19:35:10Z

src/utils/tracks.js

    trackTotalMinutesInTZ >= timeOfDayPeriod.rangeMinutesMin && trackTotalMinutesInTZ <= timeOfDayPeriod.rangeMinutesMax
  );
+
+  return period ?? TIME_OF_DAY_PERIODS[0];


return period ?? TIME_OF_DAY_PERIODS[0];

A default here means we're straight-up presenting potentially incorrect information about the time of day for a segment....that doesn't feel particularly safe to me

That one came from a test, not sure where, anyways I removed it and did several test to see if any time got no period object and indeed, there were some of them not getting set that property, so I added a validation and works great now!

JoshuaVulcan · 2025-03-03T19:36:30Z

src/utils/tracks.js

+    });
+  }
+
+  return {


This file already imports the featureCollection utility fn from turf, why not just return featureCollection(segments) ? Seems cleaner to me.

luixlive · 2025-03-03T20:23:17Z

src/selectors/tracks/index.js

-              }
-            };
-          })
+      return isTimeOfDayColoringActive


One extra comment. I just realized while working on another ticket that you added this conditional but never actually tested it. This file already has unit tests, we are just missing one test to make sure that the time of day segments are added or not based on isTimeOfDayColoringActive. Let's add it please 😄

luixlive

Not blocking this PR, I think @JoshuaVulcan has a better understanding on the reasoning behind and if he likes the approach I think it's ok too. Still, added some more comments on very minor things that I think could improve performance and code readability. Nice work, this ticket was a tricky one!

luixlive · 2025-03-04T21:51:37Z

src/hooks/useMapLayers/index.js

+  );
+};
+
+const useMapLayers = (layerConfigsBatch = [], defaultConfig = {}) => {


I still have the concern about having two configurations: the individual layer configuration and the default configuration. I feel it counter-intuitive. I'd just leave the layer configurations and the implementation can do the job of injecting common configs just before providing them to this hook. What do you think? 🤔

@gaboDev this is a good point -- while I think it is generally useful to give a "default" config plus overrides per-layer, the interfaces by which you can do this are pretty unclear and not easy to reason through. Can you improve code legibility to better-document and more plainly state what the configuration objects accept and which takes precedence?

At the end, calculating layers for segment pairs was the only place where the default config param was being used, so it was just matter of inject that global options to each layer (which was kind of already happened) so global default configs for a batch of layers was not really necessary, I think the implementator should be in charge of provide a proper layer config object per batch so We left out of the hook this responsibility

luixlive · 2025-03-04T21:55:28Z

src/hooks/useMapLayers/index.js

+  ? layerConfig.options.condition
+  : true;
+
+const shouldUpdateMapLayer = (layerConfig, map) => {


The return isn't needed and the logical expression has some unnecessary !! coercions since you are already wrapping it with one:

const shouldUpdateMapLayer = (layerConfig, map) => !!(layerConfig?.id && hasLayerCondition(layerConfig) && map.getLayer(layerConfig.id))

luixlive · 2025-03-04T21:56:50Z

src/hooks/useMapLayers/index.js

+import { MapContext } from '../../App';
+import { MAX_ZOOM, MIN_ZOOM } from '../../constants';
+
+const hasLayerCondition = (layerConfig) => layerConfig?.options?.hasOwnProperty('condition')


The name of this method implies that it will return true if the layer has a condition property (independent of its value). Maybe assertLayerCondition? And here we sill have the issue about the condition being null.

luixlive · 2025-03-04T22:03:03Z

src/hooks/useMapLayers/index.js

+          && layerConfig?.type
+          && layerConfig?.sourceId
+          && map.getSource(layerConfig.sourceId)
+          && layerConfig?.options?.condition !== false


Wait, now I'm confused, why aren't we using the new method hasLayerCondition. I like the direct comparison to false, but then why sometimes we use the method and sometimes we don't?
Also, why aren't we using shouldUpdateMapLayer here?

shouldUpdateMapLayer implies that the layer should exist which is not required here, and regarding condition, I forgot to use the method here 😄

luixlive · 2025-03-04T22:03:51Z

src/hooks/useMapLayers/index.js

+              filter,
+              before
+            } = {}
+          } = layerConfig;


nit: I feel like extracting these variables from the object just adds a bunch of unnecessary lines to the file.

At least it describes the shape of the expected configuration, which would otherwise only be intuited by reading the whole function thoroughly. 🤔

The add layer effect is way more cleaner now, this destructuring was removed from it

luixlive · 2025-03-04T22:05:51Z

src/hooks/useMapLayers/index.js

+          }
+
+          // Handle line-gradient and line-color conflict
+          if (type === 'line' && paint?.['line-gradient'] && paint?.['line-color']) {


Two things:
1- I see a console warn here, is this information that important to be printed? Will it become just noise?
2- Is this hook's responsibility to handle this "conflict"? I feel like the implementator should be careful to not add both styles if the conflict with each other, instead of deleting a random property here.

IMHO this should just be removed entirely. Resolutions in style conflicts should be fixed upstream by the implementor.

luixlive · 2025-03-04T22:10:02Z

src/hooks/useMapLayers/index.js

+          && before
+          && map.getLayer(layerConfig.id)
+        ){
+          map.moveLayer(layerConfig.id, before);


Shouldn't we check if we really need to call this method? I mean, this useEffect will be called with any change in the config objects and for every single layer with a before property it will call the moveLayer. But, do we need to? Maybe they are already well located in the layer stack right? I don't know how much this could impact performance.

Nice observation I can take a look to it

luixlive · 2025-03-04T22:11:22Z

src/hooks/useMapLayers/index.js

+      layerConfigsBatch.forEach(layerConfig => {
+        if ( shouldUpdateMapLayer(layerConfig, map) ) {
+          const { options: { minZoom, maxZoom } = {} } = layerConfig;
+          map.setLayerZoomRange(


Same thing, maybe the zoom is already in the right level, but we are not checking that. We are just iterating and calling the setLayerZoomRange for every single layer when there is a single change in the config objects. Considering that now we are supporting batches that could have dozens of layers, this may have some performance impact.

I removed both effect since where practically just increasing performance with no specific purpose

luixlive · 2025-03-04T22:12:05Z

src/hooks/useMapLayers/index.js

+  }, [map, layerConfigsBatch, defaultConfig]);
+
+  useEffect(() => {
+    const refs = layerIdsRef.current;


Still confused on why we redefine a ref into a more generic name ref

There is a linter warn about accessing a ref in a unmount callback, so just to get ride of it I declared this outside that callback, I can renaming the variable or add a disable-lint line

Don't disable the linter for that line, it exists for a reason 😅

luixlive · 2025-03-04T22:13:39Z

src/hooks/useMapSources/index.js

+    sourceConfigsBatch.forEach(sourceConfig => {
+      const source = map?.getSource?.(sourceConfig?.id);
+      if (sourceConfig?.id && sourceConfig?.data && source){
+        const timeout = window.setTimeout(() => {


bump to my original question, why do we use timeouts here?

Removed already, sorry missed that in the last iteration

luixlive

Approving just for unblocking, but please read my latest suggestions, and I'd also recommend to wait for a second pass from @JoshuaVulcan since he has more insight on the reasoning behind this approach.

JoshuaVulcan

@gaboDev there's some good feedback from @luixlive to address -- especially open questions around the efficiency of iterating long lists to support small changes on individual members of those lists.

Additionally, looks like there's some funk in the processing/rendering of the tracks themselves; namely, if I reduce my track length to a temporal period which should have 0 track points, some time-of-day track data remains drawn on the map. See here

JoshuaVulcan · 2025-03-04T23:02:02Z

src/hooks/useMapLayers/index.js

+          }
+
+          // Handle line-gradient and line-color conflict
+          if (type === 'line' && paint?.['line-gradient'] && paint?.['line-color']) {


IMHO this should just be removed entirely. Resolutions in style conflicts should be fixed upstream by the implementor.

JoshuaVulcan · 2025-03-04T23:04:10Z

src/hooks/useMapLayers/index.js

+  );
+};
+
+const useMapLayers = (layerConfigsBatch = [], defaultConfig = {}) => {


@gaboDev this is a good point -- while I think it is generally useful to give a "default" config plus overrides per-layer, the interfaces by which you can do this are pretty unclear and not easy to reason through. Can you improve code legibility to better-document and more plainly state what the configuration objects accept and which takes precedence?

JoshuaVulcan · 2025-03-04T23:05:29Z

src/hooks/useMapLayers/index.js

+              filter,
+              before
+            } = {}
+          } = layerConfig;


At least it describes the shape of the expected configuration, which would otherwise only be intuited by reading the whole function thoroughly. 🤔

JoshuaVulcan · 2025-03-04T23:07:06Z

src/hooks/useMapLayers/index.js

+  }, [map, layerConfigsBatch, defaultConfig]);
+
+  useEffect(() => {
+    const refs = layerIdsRef.current;


Don't disable the linter for that line, it exists for a reason 😅

gaboDev · 2025-03-06T16:55:12Z

Additionally, looks like there's some funk in the processing/rendering of the tracks themselves; namely, if I reduce my track length to a temporal period which should have 0 track points, some time-of-day track data remains drawn on the map. See here

This bug was related with old layers refs being still in the map while not being present in the layer config batch, I just add some logic to remove those layers when necessary

@JoshuaVulcan

JoshuaVulcan

@gaboDev approved to not block, but we have an ongoing Slack conversation on the value of maintaining both the full track line + two-point segments, vs just holding the two-point segments in memory.

JoshuaVulcan and others added 6 commits February 26, 2025 14:16

very sloppy proof-of-concept code for track time-of-day styles

76968b1

tweak:removing a few extraneous console.logs

f3349b8

WIP: code refactor, adding test coverage, connect all features to red…

3270896

…ux coloring flag

Adding batch support for useMap* hooks

4ff472f

Reseting .env

80abadf

Resseting compose file

114c3ac

gaboDev added the deploy label Mar 3, 2025

gaboDev requested review from luixlive and JoshuaVulcan and removed request for luixlive March 3, 2025 14:23

gaboDev temporarily deployed to ERA-11057 March 3, 2025 14:23 — with GitHub Actions Inactive

gaboDev requested review from AlanCalvillo and luixlive March 3, 2025 14:23

gaboDev marked this pull request as ready for review March 3, 2025 14:24

gaboDev temporarily deployed to padas-app March 3, 2025 14:24 — with GitHub Actions Inactive

gaboDev changed the title ~~ERA-11057~~ ERA-11057: track styles: track color expression Mar 3, 2025

Adding options object to useMapSource hook, removing unnecessary enab…

8687201

…led prop from source

gaboDev temporarily deployed to ERA-11057 March 3, 2025 15:42 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 3, 2025 15:42 — with GitHub Actions Inactive

Removed unused hook

d0d06c2

gaboDev temporarily deployed to ERA-11057 March 3, 2025 15:43 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 3, 2025 15:43 — with GitHub Actions Inactive

gaboDev commented Mar 3, 2025

View reviewed changes

luixlive requested changes Mar 3, 2025

View reviewed changes

Adding missing coverage to useMapLayer

244beb2

gaboDev temporarily deployed to ERA-11057 March 3, 2025 18:08 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 3, 2025 18:08 — with GitHub Actions Inactive

JoshuaVulcan requested changes Mar 3, 2025

View reviewed changes

Adding PR feedbacl

4bf2585

gaboDev temporarily deployed to ERA-11057 March 3, 2025 19:54 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 3, 2025 19:54 — with GitHub Actions Inactive

luixlive reviewed Mar 3, 2025

View reviewed changes

Adding 2nd review feedback

7d7e99d

gaboDev temporarily deployed to ERA-11057 March 4, 2025 15:57 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 4, 2025 15:57 — with GitHub Actions Inactive

gaboDev requested review from JoshuaVulcan and luixlive March 4, 2025 15:58

Solving removing layer bug

2a436ea

gaboDev temporarily deployed to ERA-11057 March 4, 2025 21:09 — with GitHub Actions Inactive

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

Gabriel López added 3 commits March 4, 2025 15:35

Turning FF off

a3202cf

Merge branch 'develop' into ERA-11057

719a44a

Turning FF on

24adc9a

gaboDev temporarily deployed to ERA-11057 March 4, 2025 21:36 — with GitHub Actions Inactive

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

luixlive reviewed Mar 4, 2025

View reviewed changes

luixlive approved these changes Mar 4, 2025

View reviewed changes

JoshuaVulcan requested changes Mar 4, 2025

View reviewed changes

Fixing track length bug, adding feedback PR

229aac1

gaboDev requested a review from JoshuaVulcan March 6, 2025 16:49

Solving merge conflicts

f083517

gaboDev temporarily deployed to ERA-11057 March 6, 2025 18:31 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 6, 2025 18:31 — with GitHub Actions Inactive

JoshuaVulcan approved these changes Mar 6, 2025

View reviewed changes

JoshuaVulcan merged commit cda7184 into develop Mar 6, 2025
3 checks passed

JoshuaVulcan deleted the ERA-11057 branch March 6, 2025 22:19

ERA-11057: track styles: track color expression #1243

ERA-11057: track styles: track color expression #1243

Uh oh!

Conversation

gaboDev commented Feb 27, 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)

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 commented Mar 3, 2025

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luixlive Mar 3, 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!

JoshuaVulcan 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!

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

gaboDev commented Feb 27, 2025 •

edited

Loading

luixlive Mar 3, 2025 •

edited

Loading