maplibre / maplibre-navigation-ios

MapLibre Navigation SDK for iOS
Other
36 stars 29 forks source link

Clean up StyleManager and respect dynamic type #65

Closed michaelkirk closed 2 months ago

michaelkirk commented 3 months ago

Description

I broke this into two commits, it might be worth reviewing the commits separately. If you'd prefer, I can open them as two separate PRs.


The first commit changes the API of the StyleManger, and consequently cleans up its internals, but it's intended to be behaviorally a no-op.

Before:

let styleManger = StyleManager()
styleManger.styles = [dayStyle] 
// or
styleManger.styles = [dayStyle, nightStyle] 

After:

let styleManger = StyleManager(dayStyle: dayStyle)
// or
let styleManger = StyleManager(dayStyle: dayStyle, nightStyle: nightStyle)

styleManager.ensureAppropriateStyle()

I'm unsure if it's a good idea to require the explicit styleManager.ensureAppropriateStyle() after initialization. Previously this logic happend in styles.didSet.

I could move it into the StyleManger.init, but editing global state feels like a weird thing to do in an initializer.


The second commit fixes a problem: changes to dynamic type were not being reflected until the style changed for some other reason. It looks like this was broken at some point (maybe in e134895db65df4501c8773350c5dbdcd163b9da4)

Before: font size does not update

https://github.com/maplibre/maplibre-navigation-ios/assets/217057/1da3e484-8852-4c88-a948-6aad8f6fad21

After: font size updates immediately

https://github.com/maplibre/maplibre-navigation-ios/assets/217057/98ec60b8-0344-4eaa-8d87-f3d1267048a3

with small font

Screenshot 2024-06-14 at 13 36 47

with big font

Screenshot 2024-06-14 at 13 36 53

--

Note: I'm currently unable to test on CarPlay. Apparently you need some special entitlement from Apple even to run your app on the CarPlay simulator (is that right?!).

Even though I don't currently plan to use CarPlay in my own app, I've applied for the entitlement so I can test changes to like this that might brush up against it.

boldtrn commented 3 months ago

Note: I'm currently unable to test on CarPlay. Apparently you need some special entitlement from Apple even to run your app on the CarPlay simulator (is that right?!).

I saw you slack message. It seems like you were able to get CarPlay to work, were you able to check this PR?

michaelkirk commented 3 months ago

I did get the car play entitlement, but I haven’t had time yet to figure out how to launch the car play view controllers. I’ll figure that out next week. That’d be something good to add to the example app once that’s merged.