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

Allow title label font to be set #94

Closed
wants to merge 5 commits into from

Conversation

ryanwaggoner
Copy link

No description provided.

@intonarumori
Copy link
Contributor

intonarumori commented Jan 10, 2017

Hey @ryanwaggoner , thanks for taking the time to contribute.

I'm afraid we cannot accept the PR in the current form.
If you look at the files changed tab it's evident that you have switched to space based tabbing.

It's impossible to see the real changes this way, basically the whole file is one block of change.

Would you mind fixing this issue?

Edit: sorry, the other way around, you are using tabs while the project uses spaces.

@ryanwaggoner
Copy link
Author

Fixed

@intonarumori
Copy link
Contributor

@ryanwaggoner Thanks.

One more thing, let's use titleFont in createTitleLabel like we do with titleColor.

@ryanwaggoner
Copy link
Author

Ah yeah, I had that originally, and missed it when going back to spaces. One min.

@ryanwaggoner
Copy link
Author

Done

@intonarumori
Copy link
Contributor

@ryanwaggoner Thanks.

Do you think we could get rid of the change in the project file? It seems you just disabled tab based white spacing for that one file.
It would be nice to get that removed and really just have the changes in the SkyFloatingLabelTextField class.

@intonarumori
Copy link
Contributor

@ryanwaggoner Great, many thanks.

I have to double check what happens if you change the titleFont to a smaller or larger one, and how the title label font height might affect the placement of other view components.

This might take a while so I will get back to you with my review and hopefully PR approval.

@intonarumori
Copy link
Contributor

@ryanwaggoner
I've been looking at our test cases and just realised that you can set the title font already by using textField.titleLabel.font = yourCustomFont. Maybe that solves your current use cases and you can use it straight away.

I have to do some more investigations to determine if leaving titleLabel exposed or adding the titleFont property is a better choice in the long term. I get back to this PR with my findings.

@Fawxy
Copy link

Fawxy commented Jan 11, 2017

It's nice having it as an @IBInspectable

@@ -74,6 +74,13 @@ open class SkyFloatingLabelTextField: UITextField {
}
}

/// A UIFont value that determines font of the title label
@IBInspectable open var titleFont:UIFont? = UIFont.systemFont(ofSize: 13.0){
Copy link
Contributor

Choose a reason for hiding this comment

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

Pelase put a space before { on this line.

@k0nserv
Copy link
Contributor

k0nserv commented May 23, 2017

Closing as #141 was merged

@k0nserv k0nserv closed this May 23, 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.

4 participants