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

Add iOS-only systemItem prop for buttons in Navigator.Config #102

Merged
merged 1 commit into from
Jul 14, 2017

Conversation

nfcampos
Copy link
Contributor

@nfcampos nfcampos commented May 3, 2017

Hello :)

This PR introduces a systemItem property for Button in Navigator.Config to allow usage of UIBarButtonSystemItem for buttons in the navigation bar (iOS-only, not sure if there is an equivalent in android).
This provides easy access to some very common icons and localised buttons on iOS, such as add, edit, done, search, etc, (full list available here https://developer.apple.com/reference/uikit/uibarbuttonsystemitem)
The only one I didn't implement fixedSpace as it requires a width parameter that I'm not sure how to expose.

Changes

  • Implement systemItem property on Button
  • Updated the docs
  • Added a new screen to the example app showing some of the ways the NavigationBar can be configured (I'll see if I find some time to update this screen with even more entries in another PR)

An example with a right button using add:
screen shot 2017-05-03 at 22 50 29

@@ -192,6 +200,36 @@ func barButtonStyleFromString(_ string: String?) -> UIBarButtonItemStyle {
}
}

func barButtonSystemItemFromString(_ string: String?) -> UIBarButtonSystemItem? {
switch string {
case .some("done"): return .done
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just check like: case "done":? Might take out the verbosity here

@benkraus
Copy link
Contributor

benkraus commented Jun 1, 2017

Dang, I just merged something else. :| Can you rebase so we can get this merged?

style: UIBarButtonItemStyle,
enabled: Bool?,
tintColor: UIColor?,
titleTextAttributes: [String: Any]?
) {
if let title = title {
self.init(title: title, style: style)
} else if let barButtonSystemItem = barButtonSystemItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a side note - in iOS, it's pretty much the fact that you can't have a mixed bar button type of a system vs one with a title/image. In the future, I'd love to support setting both a title AND an image which can be done through the customView initializer on UIBarButtonItem. That being said, it'd make sense to have this as the very first case, then if it isn't a system item, go on to title/image etc. Not a big deal now, yet. We can either bump this up now, or perhaps do it in the future, no big deal either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved up

@henrikra
Copy link

@nfcampos Are you still on this?

@nfcampos
Copy link
Contributor Author

@benkraus @henrikra totally forgot about this, i've rebased now

style: UIBarButtonItemStyle,
enabled: Bool?,
tintColor: UIColor?,
titleTextAttributes: [String: Any]?
) {
if let title = title {
if let barButtonSystemItem = barButtonSystemItem {
self.init(barButtonSystemItem: barButtonSystemItem)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this new convenience initializer? Can we just call super.init(barButtonSystemItem: barButtonSystemItem, target: self, action: #selector(barButtonItemPressed))?

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 was just following the style of the other convenience initialisers. I personally don't see the value, but I didn't want to change it in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, it's really just cleaner is all. Not a big deal. I say we merge!

@benkraus benkraus merged commit 3f91a95 into airbnb:master Jul 14, 2017
@benkraus
Copy link
Contributor

We should get this project set up on a CI system of some sorts...

@nfcampos nfcampos deleted the system-item branch July 14, 2017 21:21
@nfcampos
Copy link
Contributor Author

yeah definitely

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