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

[WIP] Fix FillDesignable bugs #462

Merged
merged 4 commits into from Apr 30, 2017
Merged

[WIP] Fix FillDesignable bugs #462

merged 4 commits into from Apr 30, 2017

Conversation

SD10
Copy link
Member

@SD10 SD10 commented Apr 26, 2017

Current Changes (Work In Progress):

  1. Fixes isOpaque flag that is never getting set because it defaults to false

  2. Sets backgroundColor of contentView for UICollectionViewCell
    We have this behavior for UITableViewCell so I assume we want it on UICollectionViewCell

I would also like to fix #461 and complete testing before merging this in.

I figure this would be the best place to post and discuss issues rather than spam the chat over every little minor detail.

@IBAnimatableBot
Copy link

IBAnimatableBot commented Apr 26, 2017

1 Warning
⚠️ PR is classed as Work in Progress

Generated by 🚫 Danger

@tbaranes
Copy link
Member

I'm surprised this is compiling, UICollectionViewCell doesn't have a contentView as UITableViewCell. How is it working? 🤔

@SD10
Copy link
Member Author

SD10 commented Apr 26, 2017

I see a contentView property for UICollectionViewCell in the header file

@JakeLin
Copy link
Member

JakeLin commented Apr 26, 2017

@SD10 yes, it is

open class UICollectionViewCell : UICollectionReusableView {

    
    open var contentView: UIView { get } // add custom subviews to the cell's contentView

I assume what's on top of @tbaranes is why do we use contentView.layer instead of layer for AnimatableCollectionViewCell. Because we can treat AnimatableTableViewCell same as the other UIView subclasses.

@JakeLin
Copy link
Member

JakeLin commented Apr 26, 2017

@mmadjer Do you remember why did we implement configureCornerRadius differently for AnimatableCollectionViewCell?

Copy link
Member

@JakeLin JakeLin left a comment

Choose a reason for hiding this comment

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

👍

public func configureFillColor() {
if let fillColor = fillColor {
backgroundColor = fillColor
contentView.backgroundColor = fillColor
Copy link
Member

Choose a reason for hiding this comment

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

Look like this is a case we keep fillColor 😁

@mmadjer
Copy link
Contributor

mmadjer commented Apr 27, 2017

@JakeLin, because for UIColletionViewCell you have to use the layer of the contentView. Is this what you're after?

@JakeLin
Copy link
Member

JakeLin commented Apr 27, 2017

@mmadjer you are right, we should use contentView for both UICollectionViewCell and UITableViewCell.

@JakeLin
Copy link
Member

JakeLin commented Apr 27, 2017

@SD10 I tried something to eliminate the code

public protocol CellFillDesignable: class {
  var backgroundColor: UIColor? { get set }
  var contentView: UIView { get }
}

extension UITableViewCell: CellFillDesignable {}
extension UICollectionViewCell: CellFillDesignable {}

public extension FillDesignable where Self: CellFillDesignable {
  public func configureFillColor() {
    if let fillColor = fillColor {
      backgroundColor = fillColor
      contentView.backgroundColor = fillColor
    } else if let predefinedColor = predefinedColor?.color {
      backgroundColor = predefinedColor
      contentView.backgroundColor = predefinedColor
    }
  }
}

But I ended up the error like /Users/jake.lin/dev/Jake/IBAnimatable/IBAnimatable/AnimatableTableViewCell.swift:35:7: Ambiguous use of 'configureFillColor()' for AnimatableTableViewCell and AnimatableCollectionViewCell because they don't know which to use, there are two configureFillColor methods for them.

@SD10
Copy link
Member Author

SD10 commented Apr 27, 2017

@JakeLin yeah I thought about this but it's tricky. I think most solutions I came up with did more harm than good...

e.g) polluting UIKit API, a new class, changing access control.

The object graph for this project is huge... I'm trying to limit introducing new types of any kind :(

Right now my personal policy is not to introduce a new type unless it replaces two existing types. With the exception of new features of course.

@JakeLin
Copy link
Member

JakeLin commented Apr 27, 2017

@SD10 I think your solution is acceptable. let's do it like this now.

@JakeLin
Copy link
Member

JakeLin commented Apr 30, 2017

@SD10 I think we should merge this PR first, then rebase the testing/unit-testing branch from master. Then we can review the changes trunk by trunk.

@SD10 SD10 reopened this Apr 30, 2017
@SD10 SD10 merged commit bbf222d into master Apr 30, 2017
@JakeLin JakeLin deleted the BugFix-FillDesignable branch May 2, 2017 02:29
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.

backgroundColor for AnimatableStackView not set by FillDesignable
5 participants