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.

Sign up for GitHub

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

Jump to bottom

Add AdaptiveStyle based on UIFontMetrics #361

Merged
merged 6 commits into from
May 23, 2019
Merged

Add AdaptiveStyle based on UIFontMetrics #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
804ff8e
Add support for adaptive sizing based on UIFontMetrics
May 6, 2019
db72d01
Skip font metrics tests on iOS 10
May 6, 2019
898d7ad
Revert change to whitespace
May 6, 2019
094548c
Merge branch 'master' into adapt-font-metrics
chrisballinger May 22, 2019
6e542ee
Trigger CI
chrisballinger May 22, 2019
24d9d69
Fix failure when running Danger on forked PRs
chrisballinger May 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ jobs:
name: Danger
when: always
command: |
bundle exec danger
if [ -n "$DANGER_GITHUB_API_TOKEN" ]; then bundle exec danger; else echo "Skipping Danger for forked pull request."; fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

@chrisballinger aren’t forked PRs where we want Danger the most, to enforce various things that community members may not be as used to as our internal team?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ZevEisenberg Yeah I'm not quite sure what to do about that, but it definitely would not be safe to turn on CircleCI's "share secrets with forks" feature. Previously we were skipping CI builds entirely for forks, which combined with our GitHub merge protection settings prevented us from merging any forked PRs. :)

At least if there are issues running the rest of the danger CI job, the job will still fail, and we'll have access to the build artifacts, test reports, and code coverage in CircleCI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do anything with Danger without access to build secrets? Otherwise, maybe some kind of staging PR? Or can we run Danger out-of-band somehow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately Danger can't post comments to GitHub without access to $DANGER_GITHUB_API_TOKEN, and I'm not comfortable with the scope of the token if it's leaked/exfiltrated when passed to a forked PR:

We recommend giving the token the smallest scope possible. This means just public_repo, this scope is still ideally too much but this account shouldn’t have any access to other repos or organizations - so malicious use of the token is scoped to making new repos on it, or writing comments on other OSS projects. Because the token can be quite easily be extracted from the CI environment, this minimizes the chance for bad actors to cause chaos with it.

From GitHub's OAuth docs:

public_repo - Limits access to public repositories. That includes read/write access to code, commit statuses, repository projects, collaborators, and deployment statuses for public repositories and organizations. Also required for starring public repositories.

We could still run Danger "locally", which prints the results to the console, to still ensure that nothing has been broken in the Danger setup.

$ bundle exec danger pr $CIRCLE_PULL_REQUEST
...
Results:

Warnings:
- [ ] Big PR

Markdown:
## Current coverage for BonMot.framework is `82.51%`
✅ *No files affecting coverage found*

It would be interesting to set up a service that runs something similar to Danger via GitHub Action hooks, using the pre-baked danger pr markdown output from a CircleCI artifact. I know a few people out here are working on a hack project to make a Slack bot that spams you about your open PRs, perhaps we could use that as a template.

- run:
name: Upload to Codecov
when: always
Expand Down
47 changes: 41 additions & 6 deletions Sources/UIKit/AdaptiveStyle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ public enum AdaptiveStyle {
/// family of methods.
case preferred

/// Enable automatic scaling of fonts obtained using `UIFontMetrics`
/// available on iOS 11+ based on the provided `textStyle` and optional
/// `maxPointSize`. If `maxPointSize` is `nil` the font will grow unbounded.
@available(iOS 11, tvOS 11, *)
case fontMetrics(textStyle: BonMotTextStyle, maxPointSize: CGFloat?)

/// If the text is scaled above `size`, substitute the font named
/// `useFontNamed`, but using all the same attributes as the original font.
/// This style may be combined with other scaling behaviors such as `control`
Expand Down Expand Up @@ -75,6 +81,16 @@ extension AdaptiveStyle: AdaptiveStyleTransformation {
else {
print("No text style in the font, can not adapt")
}
case .fontMetrics(let style, let maxPointSize):
if #available(iOS 11, tvOS 11, *) {
let metrics = UIFontMetrics(forTextStyle: style)
if let maxPointSize = maxPointSize {
font = metrics.scaledFont(for: font, maximumPointSize: maxPointSize, compatibleWith: traitCollection)
}
else {
font = metrics.scaledFont(for: font, compatibleWith: traitCollection)
}
}
case .above(let size, let fontName):
font = pointSize > size ? font.fontWithSameAttributes(named: fontName) : font
case .below(let size, let family):
Expand Down Expand Up @@ -154,6 +170,7 @@ extension AdaptiveStyle: EmbeddedTransformation {
static let preferred = "preferred"
static let above = "above"
static let below = "below"
static let fontMetrics = "fontMetrics"

}

Expand All @@ -177,23 +194,41 @@ extension AdaptiveStyle: EmbeddedTransformation {
return [EmbeddedTransformationHelpers.Key.type: Value.body]
case .preferred:
return [EmbeddedTransformationHelpers.Key.type: Value.preferred]
case .fontMetrics(let textStyle, let maxPointSize):
var attributes: StyleAttributes = [
EmbeddedTransformationHelpers.Key.type: Value.fontMetrics,
EmbeddedTransformationHelpers.Key.textStyle: textStyle,
]
if let maxPointSize = maxPointSize {
attributes[EmbeddedTransformationHelpers.Key.maxPointSize] = maxPointSize
}
return attributes
}
}

static func from(dictionary dict: StyleAttributes) -> EmbeddedTransformation? {
switch (dict[EmbeddedTransformationHelpers.Key.type] as? String,
dict[EmbeddedTransformationHelpers.Key.size] as? CGFloat,
dict[Key.fontName] as? String) {
case (Value.control?, nil, nil):
dict[Key.fontName] as? String,
dict[EmbeddedTransformationHelpers.Key.textStyle] as? BonMotTextStyle,
dict[EmbeddedTransformationHelpers.Key.maxPointSize] as? CGFloat) {
case (Value.control?, nil, nil, nil, nil):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really love this switch statement syntax but it feels kind of clunky with a 5 value tuple (previously there were only 3). I wasn't sure how best to refactor but I'm open to suggestions if reviewers agree it should be rethought.

return AdaptiveStyle.control
case (Value.body?, nil, nil):
case (Value.body?, nil, nil, nil, nil):
return AdaptiveStyle.body
case (Value.preferred?, nil, nil):
case (Value.preferred?, nil, nil, nil, nil):
return AdaptiveStyle.preferred
case let (Value.above?, size?, fontName?):
case let (Value.above?, size?, fontName?, nil, nil):
return AdaptiveStyle.above(size: size, useFontNamed: fontName)
case let (Value.below?, size?, fontName?):
case let (Value.below?, size?, fontName?, nil, nil):
return AdaptiveStyle.below(size: size, useFontNamed: fontName)
case let (Value.fontMetrics?, nil, nil, textStyle?, maxPointSize):
if #available(iOS 11, tvOS 11, *) {
return AdaptiveStyle.fontMetrics(textStyle: textStyle, maxPointSize: maxPointSize)
}
else {
return nil
}
default:
return nil
}
Expand Down
2 changes: 2 additions & 0 deletions Sources/UIKit/EmbeddedTransformation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ internal enum EmbeddedTransformationHelpers {

static let type = NSAttributedString.Key("type")
static let size = NSAttributedString.Key("size")
static let textStyle = NSAttributedString.Key("textStyle")
static let maxPointSize = NSAttributedString.Key("maxPointSize")

}

Expand Down
53 changes: 53 additions & 0 deletions Tests/AdaptiveStyleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import XCTest
let defaultTraitCollection = UITraitCollection(preferredContentSizeCategory: UIContentSizeCategory.large)

// These tests rely on iOS 10.0 APIs. Test method needs to be updated to run on iOS 9.0
//swiftlint:disable type_body_length
@available(iOS 10.0, *)
class AdaptiveStyleTests: XCTestCase {

Expand Down Expand Up @@ -147,6 +148,58 @@ class AdaptiveStyleTests: XCTestCase {
}
}

@available(iOS 11, *)
func testFontMetricsAdaptation() {
// Test is still always run (and fails) on the iOS 10 simulator despite
// the availability annotation.
guard ProcessInfo().operatingSystemVersion.majorVersion > 10 else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to suggestions here as well. I was unable to figure out how to get Xcode not to run these tests on simulator targets running iOS 10. The @available attribute is required or the tests won't even build for iOS 10 targets, but even with the attribute the test is still always executed.

I first tried using an if #available(...) instead of checking ProcessInfo but that caused a compiler warning for a redundant availability check 🤦‍♂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Guessing it’s an Obj-C runtime issue, because dynamic runtime methods are used to figure out which tests to run, and I bet they ignore availability annotations. Probably worth a radar.

return
}

let inputFont = UIFont(name: "Avenir-Book", size: 28)!
let style = StringStyle(.font(inputFont), .adapt(.fontMetrics(textStyle: .headline, maxPointSize: nil)))
print(style.attributes)

let testAttributes = { (contentSizeCategory: BonMotContentSizeCategory) -> StyleAttributes in
let traitCollection = UITraitCollection(preferredContentSizeCategory: contentSizeCategory)
return NSAttributedString.adapt(attributes: style.attributes, to: traitCollection)
}

BONAssert(attributes: testAttributes(UIContentSizeCategory.extraSmall), query: { $0.pointSize }, float: 24)
BONAssert(attributes: testAttributes(UIContentSizeCategory.small), query: { $0.pointSize }, float: 25)
BONAssert(attributes: testAttributes(UIContentSizeCategory.medium), query: { $0.pointSize }, float: 27)
BONAssert(attributes: testAttributes(UIContentSizeCategory.large), query: { $0.pointSize }, float: 28)
BONAssert(attributes: testAttributes(UIContentSizeCategory.extraLarge), query: { $0.pointSize }, float: 31)
BONAssert(attributes: testAttributes(UIContentSizeCategory.extraExtraLarge), query: { $0.pointSize }, float: 33)
BONAssert(attributes: testAttributes(UIContentSizeCategory.extraExtraExtraLarge), query: { $0.pointSize }, float: 37)
}

@available(iOS 11, *)
func testFontMetricsWithMaxPointSizeAdaptation() {
// Test is still always run (and fails) on the iOS 10 simulator despite
// the availability annotation.
guard ProcessInfo().operatingSystemVersion.majorVersion > 10 else {
return
}

let inputFont = UIFont(name: "Avenir-Book", size: 28)!
let style = StringStyle(.font(inputFont), .adapt(.fontMetrics(textStyle: .headline, maxPointSize: 30)))
print(style.attributes)

let testAttributes = { (contentSizeCategory: BonMotContentSizeCategory) -> StyleAttributes in
let traitCollection = UITraitCollection(preferredContentSizeCategory: contentSizeCategory)
return NSAttributedString.adapt(attributes: style.attributes, to: traitCollection)
}

BONAssert(attributes: testAttributes(UIContentSizeCategory.extraSmall), query: { $0.pointSize }, float: 24)
BONAssert(attributes: testAttributes(UIContentSizeCategory.small), query: { $0.pointSize }, float: 25)
BONAssert(attributes: testAttributes(UIContentSizeCategory.medium), query: { $0.pointSize }, float: 27)
BONAssert(attributes: testAttributes(UIContentSizeCategory.large), query: { $0.pointSize }, float: 28)
BONAssert(attributes: testAttributes(UIContentSizeCategory.extraLarge), query: { $0.pointSize }, float: 30)
BONAssert(attributes: testAttributes(UIContentSizeCategory.extraExtraLarge), query: { $0.pointSize }, float: 30)
BONAssert(attributes: testAttributes(UIContentSizeCategory.extraExtraExtraLarge), query: { $0.pointSize }, float: 30)
}

func testAdobeAdaptiveTracking() {
let font = UIFont(name: "Avenir-Book", size: 30)!
let chain = StringStyle(.font(font), .adapt(.control), .tracking(.adobe(300)))
Expand Down