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

More MaskDesignable #8

Closed
JakeLin opened this Issue Dec 28, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@JakeLin
Member

JakeLin commented Dec 28, 2015

Currently, MaskDesignable only supports Circle, can support more mask like Triangle and Star.

@lexrus

This comment has been minimized.

Member

lexrus commented Jan 19, 2016

Triangle mask is easy to support but every triangle must has a direction obviously.

enum MaskType {
  ...
  case TriangleUp
  case TriangleDown
  case TriangleLeft
  case TriangleRight
}
@JakeLin

This comment has been minimized.

Member

JakeLin commented Jan 19, 2016

One difficulty I am having is to support different parameters with different options. For example, for Circle, we don't need an angle for it. But for Triangle or Star, we may need a starting degree.

I am working on Star based on your PR. Will ask your favour to review it once I submit a PR. I cam up an idea to put parameter in the MaskType string. Because IB doesn't support enum as IBInspectable's type, we have to use String then convert the String to the enum. But we can also put more value than the raw value of the enum. e.g. Star(6) for a star with 6 points. That may provide a workaround for similar situation.

@lexrus

This comment has been minimized.

Member

lexrus commented Jan 19, 2016

Yeah. I were thinking about using associated types but fail back to strings at last.
There's another workaround:

#if TARGET_INTERFACE_BUILDER
  @IBInspectable public var maskType: String = ""
  @IBInspectable public var triangleDirection: String = ""
  @IBInspectable public var starPoints: Int = 0
#else
  public var maskType: MaskType = .None
#endif
...

enum TriangleDirection {
  case Up, Right, Down, Left
}

typealias StarPoints = UInt

enum MaskType {
  case None
  case Circle
  case Star(StarPoints)
  case Triangle(TriangleDirection)
}
@JakeLin

This comment has been minimized.

Member

JakeLin commented Jan 19, 2016

I really like your idea for

#if TARGET_INTERFACE_BUILDER
  @IBInspectable public var maskType: String = ""
#else
  public var maskType: MaskType = .None
#endif

I will try to replace all String to actual enum in one PR. But will introduce breaking changes to the API when we use them in code. Thanks again.

@JakeLin

This comment has been minimized.

Member

JakeLin commented Jan 19, 2016

For triangle direction, we already have a property called rotate in RotationDesignable, I steal this idea from Sketch. Unfortunately, it doesn't render properly in Interface Builder.

And other concern I have is we will add some non relevant properties showing in Interface Builder. For example, @IBInspectable public var triangleDirection: String = "" will only be used when we set maskType to .Triangle. And we may have other property called starPoints for .Star. It is what we are doing for animationType. Some properties like repeatCount can only be used for certain types of animations like Pop, Shake or Squeeze. One thing I want to do is to keep the configuration panel as clean as Sketch. But all elements in Interface Builder are semantic. In Sketch, they are all shapes. IB elements require more settings than Sketch's.

I am open to any suggestions because I am still finding the balance to that and don't mind to introduce breaking changes in early stage. Let's break, fix and move forward faster.

Thank you very much again, I am learning from you every day.

@JakeLin

This comment has been minimized.

Member

JakeLin commented Jan 20, 2016

@lexrus I found out this solution doesn't work.

#if TARGET_INTERFACE_BUILDER
  /**
    String value of predefined Animation Type, all supported types are in `AnimationType` enum
  */
  var animationType: String? { get set }
#else
  /**
   Predefined Animation Type, all supported types are in `AnimationType` enum
   */
  var animationType: AnimationType? { get set }
#endif

It renders in the Storyboard properly, but it can't enable the animation at runtime. Because the compiler treat animationType as enum AnimationType? instead of String? and that is unset from Interface Builder. It may need Xcode team to fix this issue to support enums inherited from String or other types.

@JakeLin

This comment has been minimized.

Member

JakeLin commented Jan 20, 2016

@JakeLin

This comment has been minimized.

Member

JakeLin commented Jan 21, 2016

@tbaranes

This comment has been minimized.

Member

tbaranes commented Feb 6, 2016

From the #59, I like @JakeLin's idea of Wave(up,40,10), but I'm afraid that will be really hard to understand in some cases. Wave(up) or Star(6) are easy to get, in these cases, this solution may be the best one. But if we go forward and read Wave(up,40,10): what is 40? What is 10? In this last case, we put a lot of importance in the documentation, and blind tests. Maybe it will put too much steps to use?

Another solution could be to create more views that inherit of AnimatableView:

  • AnimatableWaveView which add offset,width, anddirection` designable properties
  • AnimatableStarView which add nbOfbranches...
    I suppose to respect the current interface, we should more play on the designable, but how to make them more specific?

EDIT: Following the thread, shouldn't we renamed the issue to be more explicit?

This was referenced Feb 6, 2016

@JakeLin JakeLin added this to the v1.2 milestone Feb 13, 2016

@tbaranes tbaranes closed this in #92 Feb 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment