feat: Simplified AOI Creation ✨ #1395
Conversation
The Mapbox map instance is a mutable object that remains unchanged after initialization. Including it in the dependency array can lead to unnecessary re-renders, infinite loops, and performance issues. Similarly, adding functions that do not change was likely done to suppress linter warnings. Removing those as well.
to ensure it only runs when aoi changed, and not on general rerenders of the aoi control
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
AliceR
requested review from
hanbyul-here,
dzole0311,
sandrahoang686,
snmln,
faustoperez and
aboydnw
|
amazing work 🥳 |
|
Thanks for pointing this out! I’ll admit I didn’t do a great job clarifying the expected behavior in the spike we did. Our goal right now is to keep the drawing functionality as simple as possible, as we don’t currently see significant value in supporting multiple polygons for scientific use cases. With that in mind, the behavior you’re seeing—where the previously drawn polygon is removed when a new one is started—is intentional. When we talk about “multiple polygons,” it could mean different things: Which of these approaches were you referring to? If the need arises, it will be straightforward to expand the drawing functionality into a more robust tool, for example, to support multi-polygon creation. The current implementation is encapsulated, so we can iterate on the drawing tool without impacting other parts of the application. The output of the drawing mode is used directly, no matter how it’s created. Changing the analysis flow to support multiple distinct polygons, up to eventually enabling choropleth mapping, would require some more thought—but it’s a very interesting direction to explore! Let me know if you think we need to address this behavior further, perhaps by adding a visual signal or confirmation. It’s a great callout! |
|
Oh, I am sorry that I didn't make my point clear. I do think it is valid that we don't support multiple 'drawn polygons' as long as we support them through file upload and presets. What I was referring to is, this specific UX process that when I click on 'drawing' again. It is not clear that the old polygon will be deleted if my I draw a new polygon and I think some visual signal that the old polygon will be gone will help the user. 
|
|
I would like to hear @aboydnw 's and @faustoperez 's opinions here, but I just want to note that what you describe has happened before: when users were drawing but then selecting an area from the dropdown, the drawing would clear without warning. |
|
I think this is good enough for now, and we can iterate on the UX in the future, assuming we will make updates to E&A with our PI objective this quarter: https://github.com/NASA-IMPACT/veda-architecture/issues/556 |
|
Great work Alice! I agree with what Hanbyul said: that the intermediate step when drawing a second shape is not very clear. Also, we need to consolidate the positions for the |
app/scripts/components/common/map/controls/hooks/use-themed-control.tsx
Outdated
Show resolved
Hide resolved
From the react documentation: If your app is fully built with React, you shouldn’t need to create any more roots, or to call root.render again. It is uncommon to call render multiple times. Usually, your components will update state instead. When you want to render a piece of JSX in a different part of the DOM tree that isn’t a child of your component (for example, a modal or a tooltip), use createPortal instead of createRoot. How to guide: https://18.react.dev/reference/react-dom/createPortal\#rendering-react-components-into-non-react-dom-nodes
There was a problem hiding this comment.
Yay, thanks for tackling aoi issue again.
I understand that you didn't want to touch atom part too much that could mess the url, but I think we can at least remove the methods that are not used anymore for clarification ex. selectedForEditingAtom. (I still owe a thought on this issue: #710 will comment about it later.)
|
|
||
| export default function useAois() { | ||
| const features = useAtomValue(aoisFeaturesAtom); | ||
|
|
||
| const aoi: Feature | null = features.slice(1).reduce((acc, feature) => { |
There was a problem hiding this comment.
Is this necessary especially when we don't allow more than one polygon? Trying to see if I am missing anything.
There was a problem hiding this comment.
Thats one of my leftover thoughts in the PR description: Remove concept of "selected" or multiple aois. I left most of the logic around syncing with hashed urls untouched, but I think this would benefit from some cleanup, now what we have only ever one (Multi-)Polygon AOI.
Do you think its worth investing the cleanup time now? I wanted to get feedback on the general approach first.
| }: DrawAOIProps): [Feature | Feature[] | null, boolean] => { | ||
| const [{ drawing, isValid }, setDrawing] = useState<DrawControlState>({ | ||
| drawing: null, | ||
| isValid: false |
There was a problem hiding this comment.
Real nitpick: It could be because I am so used to the term 'validation of' geometry, but this variable name throws me slightly off. As far as I understand, this flag is an explicit variable to save whether the drawing feature is null or not, which I don't think my brain clicks to 'valid' status.
There was a problem hiding this comment.
I am using this flag to enable the 'confirm area' button after a polygon has been drawn. Confirming is not possible while still drawing the shape.
There was a problem hiding this comment.
Yah, I found that it is used there later. (I edited my comment later in case you saw the outdated one 😓 ) my comment is more about whether the variable can be named differently.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [layerId]); // do not include mapInstance to avoid unintended re-renders |
There was a problem hiding this comment.
onClick of this line and onStyleUpdate of https://github.com/NASA-IMPACT/veda-ui/pull/1395/files#diff-91147ba31615cf8c97badd4f2294836992b581298e966ecd9a4fb3039044a348R176
Are they safe to be removed?
There was a problem hiding this comment.
Can you explain why you think it should be included in the dependency array? How/when do those functions change? 🤔
There was a problem hiding this comment.
my thought process is : The onClick prop is passed from both vector-timeseries and raster-timeseries. In vector-timeseries, the value of onClick changes based on minZoom. If onClick is not included in the dependency array of a hook, the hook may not reflect the updated value of onClick when minZoom changes.
However, the more important point is that we shouldn't have to track every dependency like this manually. The standard rule—at least as enforced by our ESLint configuration—is to include all necessary dependencies in the array, only making exceptions when necessary. But we also have to be flexible and ensure our tool reflects our needs - do you see the need to mend our es-lint configuration? or is this an exception that we need to make?
There was a problem hiding this comment.
I disabled eslint as a shortcut to keep this PR from getting too bloated, but I’m happy to revisit those changes in a follow-up!
The alternative would have been rewriting even more sections of the code. Passing objects or functions as dependencies can be an anti-pattern—“If your Effect depends on an object or a function created during rendering, it might run too often.” (React Docs).
I don’t think the definition of onClick actually changes when only its value updates, so I’m not sure it would even trigger an effect. Debugging what’s causing an update can be really tricky—how would I know about minZoom affecting things down the road?
There was a problem hiding this comment.
It seems that minZoom is defined in the dataset parameters and won't dynamically change. The value passes several memoized useCallback or useMemo on the way to the onClick. The onClick handler itself is wrapped in a useCallback... I believe this is safe to remove, but please let me know if you find a problem!
Same with onStyleUpdate in app/scripts/components/exploration/components/map/index.tsx, this is wrapped in a useCallback as well...
I see that there is a lot going on, and it is difficult to know what we are breaking. But I would take the chance here and prefer cleanup over fear of breaking things.
Co-authored-by: Hanbyul Jo <[email protected]>
There was a problem hiding this comment.
Thanks for working on this 🙌🏼 ! I already think this is a significant improvement and it looks like even more that will come iteratively. I made a few comments/questions but none blocking. After Hanbyul's questions/concerns are addressed, I think its good to merge as a first iteration with next follow-ups which I wanted to ask if you have listed? Have the follow up tickets been created? Where do we go from here after this is merged?
Another note: eslint-disable-next-line react-hooks/exhaustive-deps is used quite a bit in this PR for the dependency arrays which like we talked about this morning, tends to signify some type of memory leak or other unwanted side effects that can happen. But having this eslint-disable... as a quick solution if we only want to render something on initial only is understandable especially since we do it in other places in dashboard. This isn't ideal though and especially if we use it so much. Ideally, the workaround this is usually adding all needed dependencies but conditionally exiting in the useEffect right away before any of the actual logic is executed. Could we make a ticket to clean this up?
|
|
||
| const onTrashClick = () => { | ||
| resetAoi(); | ||
| }; |
There was a problem hiding this comment.
what is the point of having this? cant we just call resetAoi() directly?
| /> | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
⛏️ 🧹 : Further cleanup can be done by breaking these out for example: drawingMode and nonDrawingMode
| ); | ||
|
|
||
| return control; | ||
| } |
There was a problem hiding this comment.
❓ : What is this wrapper abstraction and is it really needed or can it just be a part of the AoiControl component? If we keep it, can we give it a more helpful name like AoiControlWithThemedControl?
There was a problem hiding this comment.
From a UX workflow standpoint, I think we can continue to refine this in the future. For now, I think it is an improvement.
I received this feedback from Jeanné on the two different options:
I liked that I’m asked to confirm my area, but beyond that if I change my mind and delete the area to start over, I’m not noticing any difference between the two
Which, matches my gut sense about science users. I personally like fewer buttons, but I see science users liking some more clear confirmation actions.
In my opinion, we should go forward with this option and drop the experiment in #1406
I think there is something there to use in a next iteration of this flow, but with open questions on whether it should be delete, or something else, and this feedback from Jeanné, I'd propose moving forward with the flow in this PR (1395)
(obviously my review was just on the workflow and not the code itself)
Related Ticket: Close #1289
Description of Changes
This update simplifies AOI management by removing the concept of multiple AOIs where only some are selected for analysis. Instead, there will always be a single AOI that serves as input for analysis. The AOI is now displayed using a regular LineLayer with a GeoJson Source (both react-map-gl components).
There are three methods for creating an AOI: uploading a GeoJSON file, selecting a predefined boundary from a menu (or directly from the map in a future iteration), and using the Mapbox Draw Control. By leveraging the draw control solely for generating polygons, we eliminate the need for extensive custom code to sync with the library. This simplification addresses long-standing technical debt (close #710) and resolves many issues (fix #949, fix #1093, fix #1258).
Notes & Questions About Changes
A couple of things I want to do or consider before merging:
Validation / Testing
Make sure we can tick all the boxes listed in the user stories and acceptance criteria at #1289.
Did I miss anything that is not listed?
Did I break any other places where the map components are used, e.g. compare?
Did I actually solve all the bugs listed above?
This is a big PR. Please review commit by commit - due to prettier formatting many changes can be ignored in the review :)
I am also happy to pair-review with anyone interested!