feat: Separate Marker Layer from RasterTimeSeries #1595
Conversation
hanbyul-here
changed the title
hanbyul-here
changed the title
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
I appreciate the clear separation of concerns between these two layers - it's a thoughtful architectural decision that improves maintainability.
The comment below is just some food for thought regarding the decision point for rendering...
I noticed a gap where none of the layers are rendering... is that intentional? Since you set maxzoom: minZoom, in the point layer, I wouldn't expect a gap.
| return ( | ||
| points && ( | ||
| <PointsLayer | ||
| id={id} | ||
| points={points} | ||
| zoomExtent={zoomExtent} | ||
| onPointsClick={onPointsClick} | ||
| /> | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Can you explain why the points are returned from the raster-timeseries layer?
I may be missing some context, but I think the decision of whether to display points or raster based on zoom level would make more sense at a higher architectural level, before rendering either.
It seems confusing to have one layer returned from another, and leaves the responsibility unclear. A cleaner approach would be to have a decision layer that selects the appropriate visualization method (points or raster) based on zoom level, then routes to either a dedicated points layer or raster layer.
There was a problem hiding this comment.
The points are there to indicate where the raster datasets are. When a user is on a lower zoom than the dataset's minimum zoom, the points will show up. From this context that points data is derived from raster timeseries data, I think it makes sense that points are accompanied by raster paint layer, and raster-timeseries is that decision layer to render both of those layers. It would make sense to make the minZoom/maxZoom more explicit as you suggested, and I think it would be on JSX level change (in pseudo code : points && map.minZoom < ... && <Points ... than having one more layer on top of raster timeseries. Maybe I am looking at this code too long that I am missing something? In any case, let's make it better little by little 🔨
|
@AliceR that gap has been a mystery for me too!! I shamefully never tired to figure out why that is happening :[ |
Related Ticket: #1545
Description of Changes
Separate the Marker Layer from the RasterTimeSeries. For more context: Raster Timeseries shows markers for the dataset that have minimum zoom set up higher than the current Zoom to indicate to users where the data is. (ex. NightLight data: ). This PR separates these Marker Layers so that RatserTimeSeries can eventually consist of RasterPaintLayer + MarkerLayer. I believe the Marker Layer is uncoupled enough to be used outside of the RasterTimeSeries if needed.
Validation / Testing