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

backgroundColor for AnimatableStackView not set by FillDesignable #461

Open
SD10 opened this issue Apr 26, 2017 · 12 comments · Fixed by #462
Open

backgroundColor for AnimatableStackView not set by FillDesignable #461

SD10 opened this issue Apr 26, 2017 · 12 comments · Fixed by #462

Comments

@SD10
Copy link
Member

SD10 commented Apr 26, 2017

Issue

backgroundColor for AnimatableStackView not set by FillDesignable protocol and remains nil

Apparently UIStackView is a non-drawing view so drawRect() is never called and the backgroundColor is not applied.

EDIT: This obviously affects setting the opacity as well

Possible Solutions

We can possibly solve this by:

  1. Adding a backing CAShapeLayer with the background color in layoutSubviews()
  2. Adding a backing UIView with the background color in layoutSubviews()

This is not my domain of expertise so I thought I would bounce some ideas off the rest of you.

@lastMove
Copy link
Member

I think that we shouldn't fix this issue.
the current behavior is very similar to UIKit's one.

UIStackView has a backgroundColor property but setting it has no effect. (This is the kind of problem which OO code). UIStackView is non-drawable but Apple (for some good reasons) makes it inherit from UIView Even if there some of UIView property non-compatible with UIstackView purpose.
So if we make AnimatableStackView drawable it can be confusing for new users and less compliant with UIKit.

@SD10
Copy link
Member Author

SD10 commented Apr 28, 2017

@lastMove I think that's a valid point. There is obviously a reason why Apple decided to give UIStackView this functionality. @JakeLin @tbaranes your opinions?

UIStackView is not fully tested yet, but it's possible that other drawing code is not working on it as well. Maybe we should re-evaluate this class?

@tbaranes
Copy link
Member

tbaranes commented Apr 28, 2017

I agree with @lastMove, I would follow apple opinons on this. Since FillDesignable does the same thing as the backgroundColor property, we shouldn't make special case and keep copying the behavior.

In a long term process, I would be in favor to remove that designable protocol since it doesn't add features but that's another discussion 😬

@SD10 SD10 closed this as completed in #462 Apr 30, 2017
@SD10
Copy link
Member Author

SD10 commented Apr 30, 2017

Prematurely closed. This bug (which is becoming a feature) is still under review.

@SD10 SD10 reopened this Apr 30, 2017
@JakeLin
Copy link
Member

JakeLin commented May 1, 2017

I agree with @lastMove too. UIStackView is like viewgroup in Android. It is a view container.

@phimage
Copy link
Member

phimage commented Apr 20, 2018

One way to resolve this issue is to pin an other view and copy the attributes

For instance for FillDesignable and CornerDesigable

class AnimatableStackView ...
... 

  // MARK: - Fix background
  func configureCornerRadius() {
    configureBackgroundView()
  }

  func configureFillColor() {
    configureBackgroundView()
  }

  func configureBackgroundView() {
    backgroundView.translatesAutoresizingMaskIntoConstraints = false
    backgroundView.predefinedColor = predefinedColor
    backgroundView.fillColor = fillColor
    backgroundView.cornerRadius = cornerRadius
    backgroundView.cornerSides = cornerSides

    if fillColor ?? predefinedColor?.color != nil {
      insertSubview(backgroundView, at: 0)
      backgroundView.pin(to: self)
    } else {
      backgroundView.removeFromSuperview()
    }
  }

  private var backgroundView = AnimatableView()
}

private extension UIView {
  func pin(to view: UIView) {
    NSLayoutConstraint.activate([
      leadingAnchor.constraint(equalTo: view.leadingAnchor),
      trailingAnchor.constraint(equalTo: view.trailingAnchor),
      topAnchor.constraint(equalTo: view.topAnchor),
      bottomAnchor.constraint(equalTo: view.bottomAnchor)
      ])
  }
}

screen shot 2018-04-20 at 15 38 16

@SD10
Copy link
Member Author

SD10 commented Apr 20, 2018

@phimage I’m pretty sure I read in the Apple docs that UIStackView is not meant to be styled, which is why it has this behavior. I also think adding another view could surprise some users 🤔

Sent with GitHawk

@JakeLin
Copy link
Member

JakeLin commented Apr 21, 2018

@phimage thanks for investigating that. I agree with @SD10 we probably keep the default behavior. We may add a comment to AnimatableStackView to let the user know we keep the default behavior then there is a way to support background color with a custom sub-class as you provide above. How do you think?

@phimage
Copy link
Member

phimage commented Apr 21, 2018

Yes I agree. Some doc and the issue could be closed.

The default way to do in storyboard is to add a view then inside a stackview

@SD10
Copy link
Member Author

SD10 commented Apr 21, 2018

I think we should just remove the FillDesignable protocol from UIStackView since it has no effect.

open class AnimatableStackView: UIStackView, CornerDesignable, FillDesignable, BorderDesignable,

I can submit a PR if you like

@phimage
Copy link
Member

phimage commented Apr 21, 2018

Not only FillDesignable

@SD10
Copy link
Member Author

SD10 commented Apr 21, 2018

This is true, anything that relies on drawRect() wouldn't work. So we would have to remove everything other than Animatable and RotationDesignable but I'm not sure for certain if those two work either.

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.

5 participants