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

UI updates #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

UI updates #26

wants to merge 3 commits into from

Conversation

MeteC
Copy link

@MeteC MeteC commented Feb 23, 2018

Hey there, I've made some changes for my own purposes that might be of interest in the main project.

  1. Made it more obj-c friendly by adding @objc access to various customisable properties.
  2. Buttons on the action sheet now automatically highlight on touch.
  3. Item interval spacing is customisable per action sheet.
  4. You can define a default font for the whole CustomizableActionSheetItem class to set app-wide font settings simply.

…em buttons now automatically shade on highlight. Item Interval spacing is now customisable per Action Sheet. Added default (static) font to CustomizableActionSheetItem so you can define app-wide font settings in one place.
Copy link
Owner

@beryu beryu left a comment

Choose a reason for hiding this comment

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

Thanks for using and enhancing!

I reply per items what you listed.

  1. Made it more obj-c friendly by adding @objc access to various customisable properties.

I think to should not adding @objc. Because of keeping as pure swift.
In current, it’s not problem to add @objc , but it may be disturb if we wanna use new feature of Swift in future.

  1. Buttons on the action sheet now automatically highlight on touch.

It’s nice feature.
But coding style is not equal with existing codes.
Please fix it.
(Ex. “There is no self.” “First variable name what start with _”)

  1. Item interval spacing is customisable per action sheet.

This is nice feature too.

  1. You can define a default font for the whole CustomizableActionSheetItem class to set app-wide font settings simply.

Currently, this needs is satisfied by subclassing {CustomizableActionSheetItem.
So I think it should be not merged to keep interface simpler.

* Return a darker shade of the same hue.
* @param shadeFactor set in bounds [0.0, 1.0]. Lower value = darker, default = 0.85
*/
func darkerColor(shadeFactor: CGFloat = 0.85) -> UIColor? {
Copy link
Owner

Choose a reason for hiding this comment

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

Make private this extension because of there is risk conflict with library user

get {
return _backgroundColor
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

This property can be simpler by below.

public var backgroundColor: UIColor = UIColor.white {
  didSet {
    if let highlightColor = color.darkerColor() {
      self.backgroundHighlightColor = highlightColor
    }
  }
}

fileprivate var observedButton:UIButton? = nil

private var _backgroundColor: UIColor = UIColor.white
private var backgroundHighlightColor: UIColor = UIColor.white.darkerColor()!
Copy link
Owner

Choose a reason for hiding this comment

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

Do not use force unwrap.
Force unwrap is very dangerous except @IBOutlet .

@MeteC
Copy link
Author

MeteC commented Mar 23, 2018

thanks @beryu - I'll take a look at that! :-)

@MeteC
Copy link
Author

MeteC commented Apr 17, 2018

Have made changes as requested, see what you think!

In summary:

  1. Buttons on action sheet now automatically highlight on touch
  2. Item interval spacing is customisable per action sheet.
  3. (new) Items can have upperMargin value set, which adds spacing at the top of the item. This is so you can for example have a Cancel button at the bottom slightly apart from the rest of the buttons.

Code style reviewed, extra @objc access removed, static default fonts removed.

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