[Example] Data-Driven, Anchored Styling #548
Conversation
…and comment out symbol offsetting until we get symbol anchoring working reliably.
…ring value when matching the tailPosition feature attribute.
…cted as an adjective to highlighted to prevent confusion between visibly highlighting some annotations in a different color and selecting them via user interaction.
… project structure.
…z-ordering correct across annotations. (#547)
|
|
There was a problem hiding this comment.
I'm approving although it would be good to get another reviewer too since I wrote a bunch of the code I am approving. That said, most of the code was already approved in the earlier, abandoned PR so I think it should be approved by other reviewers without any major issues.
| // swiftlint:enable all No newline at end of file | ||
| // swiftlint:enable all |
There was a problem hiding this comment.
probably shouldn't be touching generated files
| } | ||
|
|
||
| @objc(CustomAnchoredSymbolExample) | ||
| public class CustomAnchoredSymbolExample: UIViewController, ExampleProtocol { |
There was a problem hiding this comment.
App code never needs to be public since it won't be consumed by other modules. Keeping it internal unlocks additional compile-time optimizations that are not possible for public constructs.
| case center | ||
| } | ||
|
|
||
| private struct DebugFeature { |
There was a problem hiding this comment.
Can we choose a different name here? What's Debug about this feature?
There was a problem hiding this comment.
Another name is fun. It was originally named debug since it was for debugging the visualization of the annotations and "Feature" was a reserved word. Maybe "AnchoredFeature" or similar is better now.
There was a problem hiding this comment.
See #548 (comment). I think avoiding the term feature in this name would be advantageous.
| let tapGestureRecognizer = UITapGestureRecognizer(target: self, action: #selector((mapSymbolTap(sender:)))) | ||
| mapView.addGestureRecognizer(tapGestureRecognizer) | ||
|
|
||
| // Allows the delegate to receive information about map events. |
There was a problem hiding this comment.
Use of the word delegate in this comment is confusing given that this doesn't really fit the typical delegation pattern on iOS
| } | ||
| } | ||
|
|
||
| private func addFeatures() -> FeatureCollection { |
There was a problem hiding this comment.
The naming of this method implies side effects, but there aren't any. How about makeFeatures instead, following the Swift API naming conventions?
| self.finish() | ||
| } | ||
|
|
||
| // Add the label on top of the map view controller. |
There was a problem hiding this comment.
| // Add the label on top of the map view controller. | |
| // Add the label on top of the map view. |
| extension UIImage { | ||
| // Produce a copy of the image with tint color applied. | ||
| // Useful for deployment to iOS versions prior to 13 where tinting support was added to UIImage natively. | ||
| func tint(_ tintColor: UIColor) -> UIImage { |
There was a problem hiding this comment.
| func tint(_ tintColor: UIColor) -> UIImage { | |
| func tinted(with tintColor: UIColor) -> UIImage { |
tintedvstint: The latter implies mutation. The former implies the creation of a new object._vswith: without the argument label, at the point of use, it reads as if you're tinting the color rather than the image.
| // Produce a copy of the image with tint color applied. | ||
| // Useful for deployment to iOS versions prior to 13 where tinting support was added to UIImage natively. |
There was a problem hiding this comment.
Could we do some availability checks inside this method and use the built-in method when available? Then when we drop support for iOS 12 and lower someday, Xcode should lead us here with a warning and we can remove this extension entirely
| return FeatureCollection(features: features) | ||
| } | ||
|
|
||
| private func addSymbolLayer(features: FeatureCollection) { |
There was a problem hiding this comment.
nit: features implies [Feature] which is only 1 aspect of a FeatureCollection.
| private func addSymbolLayer(features: FeatureCollection) { | |
| private func addSymbolLayer(with featureCollection: FeatureCollection) { |
| } | ||
|
|
||
| @objc private func mapSymbolTap(sender: UITapGestureRecognizer) { | ||
| if sender.state == .recognized { |
There was a problem hiding this comment.
Consider converting to a guard
|
|
||
| @objc(CustomAnchoredSymbolExample) | ||
| public class CustomAnchoredSymbolExample: UIViewController, ExampleProtocol { | ||
| static let symbols = "custom_symbols" |
There was a problem hiding this comment.
Could we improve the naming of this constant? Is it an identifier?
There was a problem hiding this comment.
It also seems unnecessary for this to be static. Letting it be an instance property would clean things up at the point of use.
| var features = [Turf.Feature]() | ||
|
|
||
| let featureList = [ | ||
| DebugFeature(coordinate: CLLocationCoordinate2DMake(40.714203, -74.006314), highlighted: false, sortOrder: 0, tailPosition: .left, label: "Chambers & Broadway - Lefthand Stem", imageName: "imageLeftHanded"), | ||
| DebugFeature(coordinate: CLLocationCoordinate2DMake(40.707918, -74.006008), highlighted: false, sortOrder: 0, tailPosition: .right, label: "Cliff & John - Righthand Stem", imageName: "imageRightHanded"), | ||
| DebugFeature(coordinate: CLLocationCoordinate2DMake(40.716281, -74.004526), highlighted: true, sortOrder: 1, tailPosition: .right, label: "Broadway & Worth - Right Highlighted", imageName: "imageRightHanded-Highlighted"), | ||
| DebugFeature(coordinate: CLLocationCoordinate2DMake(40.710194, -74.004248), highlighted: true, sortOrder: 1, tailPosition: .left, label: "Spruce & Gold - Left Highlighted", imageName: "imageLeftHanded-Highlighted"), | ||
| DebugFeature(coordinate: CLLocationCoordinate2DMake(40.7128, -74.0060), highlighted: true, sortOrder: 2, tailPosition: .center, label: "City Hall - Centered Highlighted", imageName: "imageCentered-Highlighted"), | ||
| DebugFeature(coordinate: CLLocationCoordinate2DMake(40.711427, -74.008614), highlighted: false, sortOrder: 3, tailPosition: .center, label: "Broadway & Vesey - Centered Stem", imageName: "imageCentered") | ||
| ] |
There was a problem hiding this comment.
Calling the lower array featureList is confusing given that now we have Turf.Feature and DebugFeature. Maybe we could rename DebugFeature in terms of just some generic data entity and avoid using the term feature in its name
| for (index, feature) in featureList.enumerated() { | ||
| let point = Turf.Point(feature.coordinate) |
There was a problem hiding this comment.
With https://github.com/mapbox/mapbox-maps-ios/pull/548/files#r673294649, in mind, maybe this would become something like…
| for (index, feature) in featureList.enumerated() { | |
| let point = Turf.Point(feature.coordinate) | |
| for (index, dataItem) in dataItems.enumerated() { | |
| let point = Turf.Point(dataItem.coordinate) |
| var pointFeature = Feature(geometry: .point(point)) | ||
|
|
||
| // Set the feature attributes which will be used in styling the symbol style layer. | ||
| pointFeature.properties = ["highlighted": feature.highlighted, | ||
| "tailPosition": feature.tailPosition.rawValue, | ||
| "text": feature.label, | ||
| "imageName": feature.imageName, | ||
| "sortOrder": feature.highlighted == true ? index : -index] | ||
|
|
||
| features.append(pointFeature) |
There was a problem hiding this comment.
And then down here things get a lot more clear because we don't have to parse feature vs pointFeature
| options: RenderedQueryOptions(layerIds: layers, filter: nil)) { result in | ||
| switch result { | ||
| case .success(let features): | ||
| if features.count > 0 { |
There was a problem hiding this comment.
| if features.count > 0 { | |
| if !features.isEmpty { |
See docs on isEmpty for the recommended best practice
| if features.count > 0 { | ||
| guard let featureText = features[0].feature.properties["text"] as? String else { return } |
There was a problem hiding this comment.
| if features.count > 0 { | |
| guard let featureText = features[0].feature.properties["text"] as? String else { return } | |
| if let featureText = features.first?.feature.properties["text"] as? String { |
macdrevx
dismissed stale reviews from avi-c and themself
Dismissing all reviews to ensure another review without blocking this PR while I'm away.
|
Closing this PR for now. We can revisit when we prioritize adding support for stretchable UIImages. |
Pull request checklist:
Summary of changes
This PR adds a new top-level example to the Examples app. This example shows how to add a custom SymbolLayer that supports adding data-driven image annotations that support anchoring on either the left, center, or right side of the annotation image. Annotations support stretchable images that will dynamically resize based on their text contents.
This was originally submitted another PR which has been closed: #190. All feedback from that PR has been addressed here.
This also addresses the issue described here: #381. This turned out to be expected behavior when using both icon-anchor and icon-text-fit.
User impact (optional)