-
Notifications
You must be signed in to change notification settings - Fork 780
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
Conversation
Generated by 🚫 Danger |
I'm surprised this is compiling, |
I see a |
@SD10 yes, it is
I assume what's on top of @tbaranes is why do we use |
@mmadjer Do you remember why did we implement |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
😁
@JakeLin, because for |
@mmadjer you are right, we should use |
@SD10 I tried something to eliminate the code
But I ended up the error like |
@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. |
@SD10 I think your solution is acceptable. let's do it like this now. |
5a2c240
to
3156b73
Compare
@SD10 I think we should merge this PR first, then rebase the |
Current Changes (Work In Progress):
Fixes
isOpaque
flag that is never getting set because it defaults tofalse
Sets
backgroundColor
ofcontentView
forUICollectionViewCell
We have this behavior for
UITableViewCell
so I assume we want it onUICollectionViewCell
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.