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

[New] Geocode offline #288

Merged
merged 17 commits into from
Oct 31, 2023
Merged

[New] Geocode offline #288

merged 17 commits into from
Oct 31, 2023

Conversation

CalebRas
Copy link
Contributor

Description

This PR implements Geocode offline in Search and Query category.
URL to README: README

Linked Issue(s)

  • swift/issues/4643

How To Test

  • Note: As of creation, this requires swift-toolkit-daily to compile.
  • Tap a location on the map to reverse geocode.
  • Enter an address in the search bar and press "Enter" to geocode.
  • Tap the arrow button in the toolbar and select an address from the list to geocode.

Screenshots

geocode-offline

To Discuss

  • I implemented the example addresses using a toolbar button instead of with the native suggestions (.searchable(text:placement:prompt:suggestions:)) because there is a bug in iOS 17 where the suggestions don't disappear when one is selected or when "Enter" is pressed, making them useless. Related Apple Developer Forum post here.

@CalebRas CalebRas self-assigned this Oct 26, 2023
@CalebRas CalebRas changed the base branch from main to v.next October 26, 2023 23:58
@CalebRas CalebRas requested review from a team, rolson and yo1995 and removed request for a team October 27, 2023 00:02
@CalebRas CalebRas requested a review from yo1995 October 27, 2023 18:58
rolson
rolson previously approved these changes Oct 30, 2023
Copy link
Contributor

@rolson rolson left a comment

Choose a reason for hiding this comment

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

Looks good. Minor comments. I think ideally there would be some error reporting if (reverse) geocode fails. But no big deal either way. Nice sample

Shared/Samples/Geocode offline/GeocodeOfflineView.swift Outdated Show resolved Hide resolved
Shared/Samples/Geocode offline/GeocodeOfflineView.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

My suggestion would still be moving the callout placement to the view, but OK if the other reviewers agree to keep it as-is. Other than that the changes look good.

@CalebRas CalebRas requested a review from yo1995 October 31, 2023 00:09
@CalebRas CalebRas requested a review from rolson October 31, 2023 00:09
@CalebRas CalebRas merged commit 0a9a851 into v.next Oct 31, 2023
1 check passed
@CalebRas CalebRas deleted the Caleb/New-GeocodeOffline branch October 31, 2023 20:14
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.

None yet

4 participants