-
Notifications
You must be signed in to change notification settings - Fork 20
feat(#257): Add geopoint with maps and placement-map appearances #536
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: f85e723 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@garethbowen if you have a chance, can you have a look? It's also okay later, too. The PR will be more ready tomorrow. (I'll DM you a video with the progress, since it has my location) Pending:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I haven't tried running the code yet so these are just readability comments. I'll give it a try soon and review again.
packages/web-forms/src/components/common/map/useMapInteractions.ts
Outdated
Show resolved
Hide resolved
packages/web-forms/src/components/common/map/useMapInteractions.ts
Outdated
Show resolved
Hide resolved
packages/web-forms/src/components/common/map/useMapViewControls.ts
Outdated
Show resolved
Hide resolved
packages/web-forms/tests/components/common/map/createFeatureCollectionAndProps.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran the code today on my dev machine (firefox, linux, desktop) and have some suggestions...
- I clicked the "zoom to current location" button, which was way off (unsurprising for a desktop), so I started scrolling around, but before I could get to my location, the map panned back to the browser provided GPS co-ords. This happens reliably - within a few seconds it always sends me back. My guess is that there's an event firing over and over again.
- If you click "zoom to current location" it takes some time (as expected), and when it completes, you click "zoom to current location" on the second map, it takes just as long again. I'm assuming the gps is local to the widget and not shared globally. It might be worth having a central service to cache the GPS location with some timeout so services can share this as it can take potentially a long time, and also can use a lot of battery. This is probably out of scope (most forms won't have multiple maps?) so feel free to split this out as a separate issue.
- The map starts in New York which is a long way away from my current location. That means when my location is found the map has to scroll a long way over map tiles that it hasn't yet downloaded. The effect of this is a rather jarring flashing between blue of the Pacific Ocean and the grey of the not yet loaded tiles. Downloading and rendering these tiles is wasteful, and the visual impact is inelegant (and flashing isn't good for some users!). It might be more useful to do a slight pan (start from 1km away) or a small zoom (start from zoomed out 2x and zoom in to the usual). That way we're downloading tiles that are actually useful, and the visual effect is smoothed.
- The spinner and message when detecting location is great, but because you're allowed to interact with the map during the detection period you may do that and then be whisked away to your location without warning. What about using the overlay that is shown for a required field as the "getting location" splash, which disables the UI which it's fetching? You could add a cancel button if the user gets bored and wants to manually find their location. This would work well with number 3 above.
I'll test on mobile (which with an actual GPS unit will be more representative) another day but that's probably enough to be getting on with!
|
Lots of great feedback. I wanted to quickly jump on on these points..
I really like this idea if it isn't too much work!
I think this is worth testing with users. Our current assumption is that users may want to interact with the map while their location is being detected. I agree it can feel disorienting to be suddenly moved, but we’ve been assuming users prefer to keep the map visible so they can confirm they’re in the right spot as they wait. Agreed adding the cancel button could be really useful here. Maybe we layer that on later? Not sure how much work that would be. |
|
Great feedback @garethbowen 🤩 Thanks!
Fixed! This is the original implementation, but I accidentally dropped it when moving code around. 🤦🏽♀️
Good idea! I think I need to fix some code to avoid side effects between questions that are in different states. I will create a ticket to address this later. Edit: This is the ticket
Agreed, on Monday I'll take a closer look to figure out how to do that easily, and post back the result. Not sure, how often users will have points so far away from their current location.
@alyblenkin agreed, I'll leave it like this for now, and see user feedback. |
| watch( | ||
| () => userCurrentLocation.value, | ||
| (newLocation) => { | ||
| const canCenterView = !userCurrentLocationFeature.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we'd stop watching location once we have found it to a sufficient accuracy, as it's quite wasteful to run, especially if we're going to ignore the result anyway. Why keep the watch running?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see what you're saying. It was requested to add a watch and update the map. Some users might move around (walking or driving) and want to see their current location on the map. It's up to the user to decide when the location is good enough and save it. Saving the location and scrolling the question out of the viewport will stop the watch, which helps free up some resources.
| <span>–</span> | ||
| <!-- TODO: translations --> | ||
| <span>Remove selection</span> | ||
| <span>Remove <span class="desktop-only">selection</span></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth finding more fluid solution, because once we implement translation you won't know how long the string is going to be, or which part of it should be chopped on mobile view. My preferred solution is to handle it by allowing the button to drop to the next line, or switching to an icon if we can find one that works. The other option is to have two translations (one for desktop and one for mobile) but that's putting more work on translators as they'll need to test both resolutions to make sure every language renders correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we've looked at these options before. The icons aren't descriptive enough and need labels, ideally in a single line. I included two translations (for desktop and mobile), which require a bit more effort from the translators, but it's a one-time task.
This is fixed, here are the videos about the implementation |
|
I had a look in mobile today (Android, Firefox) and everything looks consistent. One thing I noticed with the long hold behaviour, is after the 1 second wait, the map starts moving, and only when the animation is complete does the location icon move. It's not clear to the user that the new value is set and I think it would be more satisfying if we could move the icon, and then start the pan animation. Does that make sense? Other than that my testing is complete. Is there something else in particular you'd like me to try out? If not, I'll wait for you to finish up and request a review. |
|
Thanks @garethbowen! I've addressed the last feedback; it was just a settings issue that needed to be turned on. It's now ready for review. I'll be adding the new e2e tests in a fresh PR I’d like to extract the visual tests into their own suite and run them in a container. So, we can avoid those discrepancies that appeared in the snapshots a few weeks ago (macOS vs Linux). I'm sending this work to QA tomorrow, I'll prepare the server. |
|
@garethbowen I'll improve the edit experience for Geopoint with the You can proceed with the review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
One nit inline.
| if (props.reservedProps) { | ||
| return props.orderedExtraProps.get(props.reservedProps[ODK_VALUE_PROPERTY]) ?? []; | ||
| } | ||
| return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this one liner...
| if (props.reservedProps) { | |
| return props.orderedExtraProps.get(props.reservedProps[ODK_VALUE_PROPERTY]) ?? []; | |
| } | |
| return []; | |
| return props.orderedExtraProps?.get(props.reservedProps[ODK_VALUE_PROPERTY]) ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just remembered, it's because TypeScript is throwing some errors, and to make it fit in one line, it would need to be:
props.orderedExtraProps.get(props.reservedProps?.[ODK_VALUE_PROPERTY] ?? '') ?? [];
Then I thought it was ugly and switched it using an IF block.
Closes #257
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Verified test plan
The e2e tests are passing
Why is this the best possible solution? Were any other approaches considered?
Layer
Modes
Modules
MapBlockVue component's main responsibility is UI. Keeping minimal logic.useMapBlockis the main map manager, in charge of orchestration, and to expose all functions forMapBlockVue component. I will import other helper modules (useMapFeatures, useMapInteractions, useMapViewControl).useMapFeaturesis in charge of loading, saving and selecting features.useMapInteractionsis in charge of dragging, long press, click events (select).useMapViewControlis in charge of centering the view, watch user location.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
See the test plan and the project pointed out by Szymon for forms.
What's changed