-
Notifications
You must be signed in to change notification settings - Fork 26
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
Feat/ios/dynamic navigation view #160
Conversation
apple/Sources/FerrostarMapLibreUI/Extensions/MapViewCamera.swift
Outdated
Show resolved
Hide resolved
apple/Sources/FerrostarMapLibreUI/Views/LandscapeNavigationView.swift
Outdated
Show resolved
Hide resolved
apple/Sources/FerrostarMapLibreUI/Views/LandscapeNavigationView.swift
Outdated
Show resolved
Hide resolved
apple/Sources/FerrostarMapLibreUI/Views/LandscapeNavigationView.swift
Outdated
Show resolved
Hide resolved
apple/Sources/FerrostarMapLibreUI/Views/DynamicallyOrientingNavigationView.swift
Outdated
Show resolved
Hide resolved
I checked the failed snapshot test diff & it was just a bit of the text rendering/alias on the speed limit sign. Probably need to add a percentage diff allowance here, but we can deal with that later. |
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.
Just some minor comments; this looks fantastic in testing though, so I'll go ahead and merge!
public var topCenter: (() -> AnyView)? | ||
public var topTrailing: (() -> AnyView)? | ||
public var midLeading: (() -> AnyView)? | ||
public var bottomTrailing: (() -> AnyView)? |
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.
Do we need to add these to the constructor? Or I guess there's an extension so maybe the idea is that you don't need to override(?) these very often so it's easier to just leave it to extension methods?
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.
Nope. To keep the constructors sane, I think these only get modified exclusively through the extension view modifier style.
public var topCenter: (() -> AnyView)? | ||
public var topTrailing: (() -> AnyView)? | ||
public var midLeading: (() -> AnyView)? | ||
public var bottomTrailing: (() -> AnyView)? |
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.
Same question as above; different class ;)
Closes #39 (not directly, but I think it safely wraps up the discussion)