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

UIAppearance implementation #118

Closed
wants to merge 7 commits into from
Closed

Conversation

mwfire
Copy link
Contributor

@mwfire mwfire commented Mar 29, 2017

Hey guys,

With this PR I have added the ability to use UIAppearance for global style settings. I recommend to create a new base branch to merge and review this PR, I might have missed something. I added an example and tests. However, I have neither updated the readme nor have I bumped the version.

Let's see if that's of any use for you ;)

Cheers
Martin

@k0nserv
Copy link
Contributor

k0nserv commented Mar 30, 2017

Brillaint work @mwfire. Thanks for the PR, I'm gonna have a look at it later today

Copy link
Contributor

@k0nserv k0nserv left a comment

Choose a reason for hiding this comment

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

Could you also add an entry to CHANGELOG.md under the master section crediting yourself and referencing this PR?

/// The value of the title disappearing duration
open var titleFadeOutDuration:TimeInterval = 0.3
dynamic open var titleFadeOutDuration:TimeInterval = 0.3
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't done any UIAppearance customization with Swif, but isn't there a need for a marking such as UI_APPEARANCE_SELECTOR in Objective-C to inform the run time that this can be customized via UIAppearance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annotating properties withdynamic is sufficient to support UIAppearance in Obj-C as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Neat :)

@k0nserv k0nserv self-assigned this Apr 5, 2017
@mwfire
Copy link
Contributor Author

mwfire commented Apr 6, 2017

Sry, my version of changelog was not up-to-date. :/

Copy link
Contributor

@k0nserv k0nserv left a comment

Choose a reason for hiding this comment

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

Could you please also replaces the top comment in all new source files with this https://github.com/Skyscanner/SkyFloatingLabelTextField/blob/master/Sources/SkyFloatingLabelTextField.swift#L1-L7

@k0nserv
Copy link
Contributor

k0nserv commented Apr 10, 2017

Thanks @mwfire I'm gonna close this PR and perform the merge outside of git so I can clean up some of the commits and resolve the conflicts

@k0nserv k0nserv closed this Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants