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

Content insets and more modifiers #28

Merged
merged 16 commits into from
Mar 19, 2024

Conversation

ianthetechie
Copy link
Collaborator

Mostly toward supporting navigation use cases, but fixed up a lot of things along the way, including adding the first batch of chainable modifiers via extension methods.

@ianthetechie ianthetechie requested a review from Archdoog March 12, 2024 06:57
Comment on lines -29 to -36
public init(
styleURL: URL,
constantCamera: MapViewCamera,
@MapViewContentBuilder _ makeMapContent: () -> [StyleLayerDefinition] = { [] }
) {
self.init(styleURL: styleURL,
camera: .constant(constantCamera),
makeMapContent)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Potentially controversial breaking change: I'm removing this since it's not that hard to do this yourself (just a few more characters). The annoyance of having to keep the convenience initializers in sync with the main one will get annoying over time, so I'm of a mind to remove this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @hactar since I know you're a somewhat active user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks - not using that one - and don't worry about me, I am aware that I am using a library at version 0.0.6, I'm expecting to be hit with a lot of breaking changes 😄

"CameraState.centered(onCoordinate: CLLocationCoordinate2D(latitude: 12.3, longitude: 23.4)"
)
let state: CameraState = .centered(onCoordinate: coordinate)
XCTAssertEqual(state, .centered(onCoordinate: coordinate))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that I switched all these unit tests to snapshot tests; will keep better separation between code and test fixtures, and makes it easier to add new cases. All we're doing is ensuring we catch changes anyways.

@ianthetechie
Copy link
Collaborator Author

Had to fix up a few things after doing some local integration with Ferrostar, but it's looking solid now; ready to merge after a review.

Copy link
Collaborator

@Archdoog Archdoog left a comment

Choose a reason for hiding this comment

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

One overarching theme we need to get a handle on is our DispatchQueue/actor setup. I feel like most of our issues in this library can be solved by adding @MainActor to the encapsulating class since it's mainly UI centric (E.g. probably the coordinator itself in addition to what I flagged).

With ferrostar we probably need to dive into the swift 6 concurrency stuff a bit more broadly.

Sources/MapLibreSwiftUI/StaticLocationManager.swift Outdated Show resolved Hide resolved
Sources/MapLibreSwiftUI/MapViewCoordinator.swift Outdated Show resolved Hide resolved
Sources/MapLibreSwiftUI/MapView.swift Outdated Show resolved Hide resolved
@ianthetechie ianthetechie merged commit d330665 into main Mar 19, 2024
4 checks passed
@ianthetechie ianthetechie deleted the content-insets-and-more-modifiers branch March 19, 2024 02:42
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.

3 participants