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

refactor: using alertdialog versus popup #1156

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

clintonlunn
Copy link
Collaborator

@clintonlunn clintonlunn commented Jul 4, 2024


name: Pull request
about: Create a pull request
title: ''
labels: ''
assignees: ''


What type of PR is this?(check all applicable)

  • refactor
  • feature
  • bug fix
  • documentation
  • optimization
  • other

Description

Related Issues

Issue #1145

What this PR achieves

Screenshots, recordings

image

Notes

Copy link

vercel bot commented Jul 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
open-tacos ✅ Ready (Inspect) Visit Preview Sep 20, 2024 11:05pm

@clintonlunn
Copy link
Collaborator Author

@vnugent Looks like I might be getting the same errors as the last pr, preventing the preview from deploying. Not necessarily done with this pr or ready for review, but is there anything I can do to get the preview working?

@@ -0,0 +1,67 @@
import React, { useState, cloneElement } from 'react'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to give an alternative approach to using the alertdialog also. I was curious about how to make a custom control and also use the control positions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vnugent sorry I kind of let this pr sit for several weeks. Do you have any input on using a custom control versus the AlertDialog component like we had originally discussed? I'm fine either way, I have always been curious about how to make a maplibre custom control, so I explored doing that.

in addition, I'm not sure why the preview is not working. I see the CI jobs are showing that docker-compose isn't found.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use the existing AlertDialog. We should do UI cleanup/overhaul at some point.
The Vercel preview works for me. The docker-compose GitHub action may be broken. We can ignore it for now as long as Vercel build is green.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vnugent sounds good, I'll remove the custom control and use the AlertDialog component.

In addition, I can't promise a time commitment (it's about to be climbing season 😄), but I would like to be involved in the ui cleanup effort.

@clintonlunn clintonlunn marked this pull request as ready for review September 11, 2024 18:12
@vnugent
Copy link
Contributor

vnugent commented Sep 15, 2024

@clintonlunn thanks for the fix. I did a quick test notice the picker doesn't set initial location based on the form state:

  1. From the area edit page, click on the coordinates
  2. Pick a different location. Click Confirm.
  3. Click on the coordinate again. Notice how the drop pin is set at the original location, not at what we set in step 2.

I think the fix may be having initializing the drop pin (in a useEffect() to the value in the form state

@clintonlunn
Copy link
Collaborator Author

clintonlunn commented Sep 20, 2024

@clintonlunn thanks for the fix. I did a quick test notice the picker doesn't set initial location based on the form state:

  1. From the area edit page, click on the coordinates
  2. Pick a different location. Click Confirm.
  3. Click on the coordinate again. Notice how the drop pin is set at the original location, not at what we set in step 2.

I think the fix may be having initializing the drop pin (in a useEffect() to the value in the form state

@vnugent
Thanks, that was it. Instead of passing an initialCenter prop to the coordinate map at all, we can just watch the form context for the latlngStr value and decide where to put the map pin.

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.

2 participants