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

Mask border #101

Closed
tbaranes opened this issue Feb 14, 2016 · 10 comments · Fixed by #135
Closed

Mask border #101

tbaranes opened this issue Feb 14, 2016 · 10 comments · Fixed by #135

Comments

@tbaranes
Copy link
Member

As @lexrus pointed out in #99, we currently doesn't draw well the borders when a maskType is specified.
I join Lex to say that is a really annoying miss since a new user can be disappointed by the library.

Any thought of how we can handle this issue?

@tbaranes tbaranes added this to the 1.2 milestone Feb 14, 2016
@tbaranes
Copy link
Member Author

Some potential solutions:

  • Add custom border properties to MaskDesignable, but it will a duplicates of BorderDesignable
  • Add border properties directly to configMask(), but that will break the uniformity of the configProtocol in the project
  • Access to the border layer and update it when drawing the mask, supposing that the border is always configure before the mask, or we have to call configBorder before configMask

EDIT: Option currently implemented in the PR #99, makes BorderDesignable handle the border path, if a custom mask exists in the layer. It's the best one I can think up to now.

@lexrus
Copy link
Member

lexrus commented Feb 14, 2016

Add IBAnimatable/FontAwesomeAnimatableView into Podfile, so that you can draw any icon you want. 😆

@tbaranes
Copy link
Member Author

I reopen this issue, seems mask border isn't working anymore for stars, triangle...

@tbaranes tbaranes reopened this Mar 28, 2016
@tbaranes tbaranes modified the milestones: 2.1, 1.2 Mar 28, 2016
@tbaranes
Copy link
Member Author

That bug isn't a regression, it has always been here. In fact, we doesn't support mask border when using maskTriangle(), maskStar()... it's only working by directly the maskType.

When setting a maskType, we are reloading the border:

@IBInspectable public var maskType: String? {
    didSet {
      configMask()
      configBorder()
    }
  }

Whereas when using maskTriangle(), maskStar()... we are just updating the mask without reloading the border.

The solution would be to find a way that MaskDesignable can reload it's own border when creating a new mask. Right now, I have no idea to fix that. Any suggestions?

@tbaranes
Copy link
Member Author

One solution could be to move the configBorder() directly in the configMask(), I'm not a huge fan because that means DesignableMask knows BorderDesignable, but can't think about a better solution.

I tried this solution by casting in AnimatableView, it seems to work fine, but didn't find yet a solution to make it generic which means we have to cast self to an UIView conforming to BorderDesignable in order to call configBorder().

@JakeLin
Copy link
Member

JakeLin commented Mar 28, 2016

@tbaranes I just did a test and it is working currently for AnimatableView for triangle and star.

screen shot 2016-03-28 at 10 36 13 pm

Can you confirm is the current version of `AnimatableView` working or not?

Since we use protocol oriented approach, it is better to keep MaskDesignable and BorderDesignable separate. But when we override makeType, we have to call configBorder in didSet like

@IBInspectable public var maskType: String? {
    didSet {
      configMask()
      configBorder()
    }
  }

Same as you, I don't have any better solution for that. Maybe a new protocol called MaskBorderDesignable?

I did some experiments before to have diamond conformance for Animatable protocol. For example, I have Animatable as the root protocol, PositionAnimatable and ScaleAnimatable protocols conform to Animatable, then have a SqueezeAnimatable protocol conforms to both PositionAnimatable and ScaleAnimatable protocols, but it doesn't work.

@tbaranes
Copy link
Member Author

It works when directly used in IB, or using the setter view.maskType = String(MaskType.Circle), but the border won't be drawn when using view.maskTriangle()...
That issue is due on the fact that we are doing the configBorder() directly in didSet.

Using view setter / IB:

screen shot 2016-03-28 at 14 00 47

Using public DesignableMask method:

screen shot 2016-03-28 at 14 00 34

Another solution would be to find a way to set the maskType when calling the public method of MaskDesignable, but I don't really see how we can do this.

@JakeLin
Copy link
Member

JakeLin commented Mar 28, 2016

@tbaranes clear, because maskTriangle(), maskWave() and maskStar() are the extension methods in MaskDesignable and only know maskType. That's why it can't update the borders when use view.maskStar(6).

For the user, if they want to draw a mask with borders in a view. Using view.maskType = "Star(6)" is same effort as view.maskStar(6), right? If it is true, shall private all mask****() methods?

Any scenarios, the user can only use view.maskStar(6) not view.maskType = "Star(6)"?

Because it requires architectural changes. Sometimes, we have to sacrifice some nice API to maintain cleaner architecture. But I am open to any better solution.

@tbaranes
Copy link
Member Author

Agreed. It will be nicer to privatise these methods in order to avoid that case. Gonna make a PR.

@JakeLin
Copy link
Member

JakeLin commented Mar 28, 2016

@tbaranes thanks, nice to chat with you to figure out a better solution😀/workaround😂.

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

Successfully merging a pull request may close this issue.

3 participants