Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix map controls and resolve Analyses Message issue #1205

Open
wants to merge 6 commits into
base: 902-ea-breakout
Choose a base branch
from

Conversation

dzole0311
Copy link
Collaborator

@dzole0311 dzole0311 commented Oct 21, 2024

Related Ticket: #1155 #1172

Related Next.js PR: developmentseed/next-veda-ui#7

Description of Changes

Notes & Questions About Changes

{Add additonal notes and outstanding questions here related to changes in this pull request}

Validation / Testing

  1. Verify that the AOI controls and Analyses Message work as expected in both veda-ui and the Next.js instance
  2. Verify that adding, deleting a single drawn area, or deleting all drawn areas works without issues
  3. Ensure that analyses execute as expected

@dzole0311 dzole0311 requested review from sandrahoang686 and removed request for sandrahoang686 October 21, 2024 12:32
Copy link

netlify bot commented Oct 21, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 588fd8b
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/671b6038f3906400084588b1
😎 Deploy Preview https://deploy-preview-1205--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dzole0311 dzole0311 marked this pull request as draft October 21, 2024 12:32

if (isDrawing) {
mbDraw.changeMode(DRAW_POLYGON);
if (main.loaded()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see; so the problem was that we couldn't guarantee that the map was loaded before registering mapbox-draw-related events?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the issue came from two places:

  1. We couldn’t guarantee the map was fully loaded before adding the Mapbox Draw events. So I added this check to make sure main.loaded() is true before switching modes when DrawControl is initialized
  2. There was a strange synchronization issue in the previous code that initialized useControl. Specifically control.current was undefined later in the CustomAoiControl and adding safeguards or null checks didn't fix the problem. So I ended up assigning the control instance returned by useControl directly to a variable (like const drawControl = useControl(...)) to make sure we're always using the correct and initialized control instance

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh- interesting. It will be great to figure out why 2. is happening (not a blocker, though!)

app/scripts/components/common/map/controls/aoi/index.tsx Outdated Show resolved Hide resolved
app/scripts/components/common/map/controls/aoi/index.tsx Outdated Show resolved Hide resolved

if (isDrawing) {
mbDraw.changeMode(DRAW_POLYGON);
if (main.loaded()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh- interesting. It will be great to figure out why 2. is happening (not a blocker, though!)

} else {
mbDraw.changeMode(SIMPLE_SELECT, {
featureIds: mbDraw.getSelectedIds()
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like changeDrawMode is getting called no matter what, do we want that? I'm wondering can we check for the isDrawing bool earlier - on line 330? Then this kind of makes the if/else with defaulting to simple_select obsolete but did that do anything anyways? I checked locally and if we got rid of that logic, seems like everything still works as is but I may be missing something 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original commit addressed an issue where polygons were not being selected after completing the drawing by double-clicking. I tried this and it is reproducible locally using this line.

However you're correct that the switch to simple_select mode is redundant as it seems to be managed internally by Mapbox Draw GL (note: the explicit switch might have been needed back when the fix was implemented). So this behavior makes chunks of the code unnecessary. I removed the obsolete parts and added a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants