-
Notifications
You must be signed in to change notification settings - Fork 10
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] Set up location-driven geotriggers #198
Conversation
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.
First batch of comments.
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
…nDrivenGeotriggersView.swift Co-authored-by: Ting <tchen@esri.com>
…nDrivenGeotriggersView.swift Co-authored-by: Ting <tchen@esri.com>
Co-authored-by: Ting <tchen@esri.com>
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.
Getting into the concurrency deep water! 🎣
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
…nDrivenGeotriggersView.swift Co-authored-by: Ting <tchen@esri.com>
…nDrivenGeotriggersView.swift Co-authored-by: Ting <tchen@esri.com>
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.
Here are some docs changes. I'll add more feedback on the data model soon.
Shared/Samples/Set up location-driven geotriggers/SetUpLocationDrivenGeotriggersView.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Ting <tchen@esri.com>
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.
Instead of making separate comments, I decided to stash my suggestions on a branch. Here are the compared branches: https://github.com/Esri/arcgis-maps-sdk-swift-samples/compare/Caleb/New-SetUpLocationDrivenGeotrigers...Ting/GeotriggerSuggestions?expand=1
We can discuss these suggestions in detail. Once we are all good, you can merge it into your branch locally and update the PR. Or if you want me to comment on the changes on GitHub, feel free to open a PR targeting your branch and I can add the comments on their lines.
Hi @mhdostal if you have a chance can you review this PR? Or assign to someone else using the sample reviewers alias. |
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.
Looks great, very nice work!
Description
This PR implements
Set up location-driven geotriggers
inDisplay information
category.URL to README: https://github.com/Esri/arcgis-maps-sdk-swift-samples/tree/Caleb/New-SetUpLocationDrivenGeotrigers/Shared/Samples/Set%20up%20location-driven%20geotriggers
Linked Issue(s)
swift/issues/4135
How To Test
Screenshots