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] Search with geocode #14

Merged
merged 15 commits into from
Jun 8, 2022
Merged

[New] Search with geocode #14

merged 15 commits into from
Jun 8, 2022

Conversation

yo1995
Copy link
Collaborator

@yo1995 yo1995 commented Jun 2, 2022

Description

This PR implements Search with geocode in Search category.
URL to README: https://github.com/ArcGIS/arcgis-runtime-samples-swift/tree/Ting/New-FindAddressGeocode/Shared/Samples/Search%20with%20geocode

Linked Issue(s)

  • common-samples/issues/3110

How To Test

  1. Launch the sample, search a specific address, e.g., 380 New York St
  2. See the single result pin on the map. Tap the pin to show a callout for its short address ("Match_addr")
  3. Clear the search bar. Search for a POI term, e.g., "CVS"
  4. See the 10 results on the map.
  5. Pan the map and tap "Repeat Search Here". Results should be different.

Screenshots

220602-geocode-demo.mp4

To Discuss

  1. Text did change event modifier for SearchView's search field arcgis-maps-sdk-swift-toolkit#68 Currently the callout doesn't auto dismiss when the search bar is cleared, which is diverging from the original sample. Talked to Trevor and we OK'd not to be exactly the same behavior as before.
  2. I merged the READMEs of Find address and Find place to write this README. With the help of toolkit, the How it works section is significantly simplified. Please let me know if any essential info is missing from the README!
  3. Search view will support multiple search sources in the future. See the comment below for my concern on README.

@yo1995 yo1995 self-assigned this Jun 2, 2022
@yo1995 yo1995 marked this pull request as ready for review June 2, 2022 17:59
@yo1995 yo1995 requested a review from TADraeseke June 2, 2022 18:22
@yo1995
Copy link
Collaborator Author

yo1995 commented Jun 2, 2022

@TADraeseke I included you to take a look at this quasi-design as well as other phrasing issues. Feel free to ignore if you don't have time. 🙂

@yo1995 yo1995 requested a review from clee088 June 2, 2022 18:41
clee088
clee088 previously approved these changes Jun 3, 2022
Adjust comment column width.

Co-authored-by: David Feinzimer <dfeinzimer@gmail.com>
@yo1995 yo1995 requested a review from dfeinzimer June 3, 2022 20:30
mhdostal
mhdostal previously approved these changes Jun 3, 2022
dfeinzimer
dfeinzimer previously approved these changes Jun 3, 2022
@yo1995 yo1995 dismissed stale reviews from dfeinzimer and mhdostal via ccc075a June 4, 2022 00:00
@yo1995 yo1995 requested a review from zkline101 June 4, 2022 00:25
mhdostal
mhdostal previously approved these changes Jun 6, 2022
@yo1995 yo1995 dismissed stale reviews from zkline101 and mhdostal via d7e4488 June 7, 2022 00:54
@yo1995 yo1995 requested review from mhdostal and zkline101 June 7, 2022 17:32
mhdostal
mhdostal previously approved these changes Jun 7, 2022
Copy link
Contributor

@philium philium left a comment

Choose a reason for hiding this comment

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

Not a big deal, but based on the screenshot, it looks like there is more padding on the leading edge of the search view than on the trailing edge. Is there anything that can be done about that?

Co-authored-by: Philip Ridgeway <philip.ridgeway@gmail.com>
@yo1995
Copy link
Collaborator Author

yo1995 commented Jun 7, 2022

Not a big deal, but based on the screenshot, it looks like there is more padding on the leading edge of the search view than on the trailing edge. Is there anything that can be done about that?

It seems like there is a spacer in the search view's HStack: https://github.com/ArcGIS/arcgis-runtime-toolkit-swift/blob/v.next/Sources/ArcGISToolkit/Components/Search/SearchView.swift#L119 , though I'm not sure why 🤔

@mhdostal
Copy link
Member

mhdostal commented Jun 7, 2022

It seems like there is a spacer in the search view's HStack: https://github.com/ArcGIS/arcgis-runtime-toolkit-swift/blob/v.next/Sources/ArcGISToolkit/Components/Search/SearchView.swift#L119 , though I'm not sure why

The Spacer() is there to force the Search bar and view to the right side of the display. This allows for the "Repeat Search" button below the Search results list to be centered on the display. You can add an issue to the Toolkit repo and I can revisit how it was done.

@yo1995
Copy link
Collaborator Author

yo1995 commented Jun 7, 2022

It seems like there is a spacer in the search view's HStack: https://github.com/ArcGIS/arcgis-runtime-toolkit-swift/blob/v.next/Sources/ArcGISToolkit/Components/Search/SearchView.swift#L119 , though I'm not sure why

The Spacer() is there to force the Search bar and view to the right side of the display. This allows for the "Repeat Search" button below the Search results list to be centered on the display. You can add an issue to the Toolkit repo and I can revisit how it was done.

iPhone iPad
Simulator Screen Shot - iPhone 11 - 2022-06-07 at 14 29 39 Simulator Screen Shot - iPad Pro (11-inch) (3rd generation) - 2022-06-07 at 14 31 20

Yeah, I missed that - it's not centered in the first place. I think it's fine to put it on the right.

@yo1995 yo1995 requested a review from philium June 7, 2022 21:38
@yo1995
Copy link
Collaborator Author

yo1995 commented Jun 7, 2022

Thanks for your prompt review! I'll merge this PR after samples team review tomorrow morning.

@yo1995 yo1995 merged commit 57090b3 into v.next Jun 8, 2022
@yo1995 yo1995 deleted the Ting/New-FindAddressGeocode branch June 8, 2022 16:29
@yo1995 yo1995 changed the title [New] Search with geocode [New] Search with geocode Aug 24, 2022
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

7 participants