-
Notifications
You must be signed in to change notification settings - Fork 197
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
Fixes UISegmentedControl extension crash and improves efficiency #398
Fixes UISegmentedControl extension crash and improves efficiency #398
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Would you mind reverting the tabs/spaces change?
Fixed whitespace by replacing tabs with spaces.
@chrisballinger yes, of course! Sorry about that, it looked much better in Xcode, I promise. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Would you also mind taking a look at the test failures on CI?
#else | ||
guard let string = value.key as? StyleAttributes.Key else { | ||
fatalError("Can not convert key \(value.key) to String") | ||
@nonobjc final func bon_titleTextAttributes(for state: UIControl.State) -> StyleAttributes? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the public API, would it be possible keep this the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I have reverted the changes to the signature of that method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that method is actually internal
, so if we wanted to keep that change it wouldn't affect the public API. I'm fine with either direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it looks like the tests are failing because Zev's twitter account is unreachable.
** BUILD SUCCEEDED **
Testing with `xcodebuild`.
-> BonMot (5.6.0)
- WARN | url: The URL (https://twitter.com/ZevEisenberg) is not reachable.
- NOTE | xcodebuild: note: Using new build system
- NOTE | xcodebuild: note: Building targets in parallel
- NOTE | xcodebuild: note: Using codesigning identity override: -
- NOTE | xcodebuild: note: Planning build
- NOTE | xcodebuild: note: Constructing build description
- NOTE | xcodebuild: warning: Capabilities for Signing & Capabilities may not function correctly because its entitlements use a placeholder team ID. To resolve this, select a development team in the App editor. (in target 'App' from project 'App')
- NOTE | xcodebuild: warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App')
- NOTE | xcodebuild: note: Using codesigning identity override:
[!] BonMot did not pass validation, due to 1 warning (but you can use `--allow-warnings` to ignore it).
You can use the `--no-clean` option to inspect any issue.
Exited with code exit status 1
CircleCI received exit code 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be related to #373 @ZevEisenberg
…ntrol' Conflicts: Sources/UIKit/AdaptableTextContainer.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a unit test we could write to confirm the new behavior?
@@ -147,19 +147,19 @@ extension UISegmentedControl { | |||
// `UISegmentedControl` has terrible generics ([NSObject: AnyObject]? or [AnyHashable: Any]?) on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this comment need to be updated at all in light of these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed an error inside that method. When I changed the return type back to being non-optional I forgot to default to an empty dictionary when nil. I also added a test for segmented control similar to the other UIKit tests.
As for the comment, I can't speak to whether it is still appropriate or not, although I can say in Swift 5 titleTextAttributes(for:)
returns [NSAttributedFont.Key: Any]?
.
…dded UnitTest for segmented control.
…Control extension method bon_titleTextAttributes(for:) since it is internal API.
@ZevEisenberg I commented out the social_media_url in the podspec and now all of the tests are passing. Any chance we could get this merged at some point? Thanks! |
…ntrol Fixed merge conflicts for PR #398
Fixes #397