Add AdaptiveStyle based on UIFontMetrics #361
Conversation
| dict[Key.fontName] as? String, | ||
| dict[EmbeddedTransformationHelpers.Key.textStyle] as? BonMotTextStyle, | ||
| dict[EmbeddedTransformationHelpers.Key.maxPointSize] as? CGFloat) { | ||
| case (Value.control?, nil, nil, nil, nil): |
There was a problem hiding this comment.
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.
| func testFontMetricsAdaptation() { | ||
| // Test is still always run (and fails) on the iOS 10 simulator despite | ||
| // the availability annotation. | ||
| guard ProcessInfo().operatingSystemVersion.majorVersion > 10 else { |
There was a problem hiding this comment.
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 🤦♂
There was a problem hiding this comment.
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.
|
Hey, @ndonald2! Thanks so much for the PR! I've been wanting to update BonMot's adaptive font support since it was updated in iOS, so I'm excited to look at this. I'll see what I can do on my hack day this week. |
| @@ -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 | |||
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- https://danger.systems/guides/getting_started.html#tokens-for-oss-projects
- https://danger.systems/guides/troubleshooting.html#i-want-to-only-run-danger-for-internal-branches
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.
Purpose
Adds a new
AdaptiveStyleenum case and adaptation implementation based onUIFontMetricsavailable in iOS 11+Context
The existing
AdaptiveStylemodes are all fantastic, but are in some part workarounds for UIKit's previous lack of support for adapting non-system fonts to the user's preferred content size category. This change adds a newAdaptiveStyleenum case and auto-scaling implementation compatible with the existingAdaptiveTextContainersystem which adopts theUIFontMetricsAPI for scaling any font with the same size curve used by the built-in system text styles.Guidance
Review from @ZevEisenberg or someone on the BonMot maintainers team. See inline notes for additional context.
Related to #293
PS - Hi @ZevEisenberg ! I hope things are well at Rightpoint. I've continued to be a big fan of BonMot since I parted ways with RZ.