[LMN-21] SwiftUI Map Markers #1724
Conversation
| .clipShape(MarkerShape()) | ||
| .overlay( | ||
| MarkerShape() | ||
| .stroke(Color(.statusSuccessSpotColor), lineWidth: 1) |
There was a problem hiding this comment.
nit: Should this be part of the state enum?
|
|
||
| public var body: some View { | ||
| Circle() | ||
| .stroke(Color(.surfaceDefaultColor), lineWidth: 2) |
There was a problem hiding this comment.
nit: Should we use a constant for this? To avoid magic numbers in code?
private let strokeWidth = 2
This can help when we have to later adjust the marker
|
|
||
| extension BPKPriceMapMarker { | ||
| struct MarkerShape: Shape { | ||
| let flareSize = CGSize(width: 16, height: 6) |
There was a problem hiding this comment.
nit: Is the width related to a BPKSpacing token? Should we use the BPKSpacingBase on this one?
| let flareSize = CGSize(width: 16, height: 6) | ||
| let cornerRadius = BPKSpacing.sm |
There was a problem hiding this comment.
Can we make these private?
| case .default: | ||
| return .label2 | ||
| case .focused: | ||
| return .label1 | ||
| case .viewed: | ||
| return .label2 | ||
| case .disabled: | ||
| return .label2 |
There was a problem hiding this comment.
Can we change this to:
| case .default: | |
| return .label2 | |
| case .focused: | |
| return .label1 | |
| case .viewed: | |
| return .label2 | |
| case .disabled: | |
| return .label2 | |
| case .focused: | |
| return .label1 | |
| default: | |
| return .label2 |
| .padding(.horizontal, .md) | ||
| .overlay( | ||
| RoundedRectangle(cornerRadius: .xs) | ||
| .stroke(Color(.coreAccentColor), lineWidth: 4) |
There was a problem hiding this comment.
Can we use a token for this, or declare it as a constant at the top?
| ) | ||
| .background(state.backgroundColor) | ||
| Color(.coreAccentColor) | ||
| .frame(width: 16, height: 6) |
There was a problem hiding this comment.
Can we use BPKSpacingBase for the 16px token?
The 6px seems fishy, did we double check if we can't use another token?
There was a problem hiding this comment.
yeah 6 is the number, it's not a toke that we want to 'support' for the outside world.. but i've extracted it to a constant at least so that's not sparsed around in the class
| .padding(.bottom, 6) | ||
| .padding(.vertical, 4) |
There was a problem hiding this comment.
Can these be Backpack spacing tokens?
There was a problem hiding this comment.
This doesn't look disabled, but that's more of a critique on the design...
There was a problem hiding this comment.
for the record: fixed this. figma was showing as disabled what was viewed and mixing the focused one.. all is fixed now.
…kpack-ios into lmn-21/map_marker-swiftui
| import SwiftUI | ||
|
|
||
| public struct BPKPriceMapMarker: View { | ||
| public enum State { |
There was a problem hiding this comment.
nit: can we move this State enum to a separate extension file? To keep the component 'neater'
There was a problem hiding this comment.
nit: should we flip the disabled and focused POI marker, so they align with the price marker?
Introducing the Map Markers for SwiftUI
https://www.figma.com/file/irZ3YBx8vOm16ICkAr7mB3/Backpack?type=design&node-id=8191-8904&mode=design&t=2QzKjHwIqZM3Z6si-0