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

Downsize shields in instructions in CarPlay #1868

Merged
merged 1 commit into from
Dec 1, 2018

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Nov 30, 2018

When preparing attributed instruction strings for CarPlay, resize any text attachment image taller than 16 points, preserving the aspect ratio.

Here’s a before and after:

before

after

Shields on the phone are unaffected:

after-phone

Fixes #1867 and speculatively fixes #1866. A more correct fix for #1866 would require adjustments to the constraints in ExitView and GenericRouteShield.

/cc @mapbox/navigation-ios

@1ec5 1ec5 added bug Something isn’t working topic: instructions CarPlay Bugs, improvements and feature requests on Apple CarPlay labels Nov 30, 2018
@1ec5 1ec5 added this to the v0.26.0 milestone Nov 30, 2018
@1ec5 1ec5 self-assigned this Nov 30, 2018
Copy link
Contributor

@frederoni frederoni left a comment

Choose a reason for hiding this comment

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

Great catch

@@ -316,6 +316,13 @@ public class CarPlayNavigationViewController: UIViewController {
return CGRect(x: 0, y: 0, width: widthOfManeuverView, height: 30)
}

// Over a certain height, CarPlay devices downsize the image and CarPlay simulators hide the image.
let maximumImageSize = CGSize(width: .infinity, height: shieldHeight)
Copy link
Contributor

@frederoni frederoni Nov 30, 2018

Choose a reason for hiding this comment

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

IIRC, the max height of an attachment in an attributed string drawn by a UILabel should be font.size.height + font.ascender + font.descender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that maximum height a best practice or a hard limit? In my testing, I kept seeing a hard 16-point height limit, but is there a way to change the font size in CarPlay? Perhaps an accessibility setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, none of the accessibility settings on iOS affect the font size of the instructions in CarPlay. CarPlay units do have their own settings (such as the left/right layout setting), but I haven’t seen a setting about the font size in the units I’ve used so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hard limit before image gets cropped or scaled depending on other attributes. I thought we had some leeway but, if there seem to be a 16p limit, let's stick with that.

When preparing attributed instruction strings for CarPlay, resize any text attachment image taller than 16 points, preserving the aspect ratio.
@1ec5 1ec5 force-pushed the 1ec5-carplay-shield-size-1867 branch from 424c414 to bdc00d3 Compare December 1, 2018 02:08
Copy link
Contributor

@JThramer JThramer left a comment

Choose a reason for hiding this comment

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

Well, the code changes look good to me, but MFW while looking at this PR:

Hoo boy!

@1ec5 1ec5 merged commit 4646fe7 into master Dec 1, 2018
@1ec5 1ec5 deleted the 1ec5-carplay-shield-size-1867 branch December 1, 2018 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn’t working CarPlay Bugs, improvements and feature requests on Apple CarPlay topic: instructions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shields missing in CarPlay simulator Exit number shields look blurry in CarPlay
3 participants