Updated Divider as UIView #1262
Conversation
|
On initial glance, this looks great. @adamdahan will comment further, and thank you @brennobemoura for sharing this with us. |
|
@brennobemoura Do you have any sample code for how you animate the divider which cause the original problem mentioned in ticket: #1252? |
|
I will search here. The problem happened around March or April and since them I write this version for Divider and it works. I remember to use the Skeleton view and when it's stops animating the divider was in wrong place or in a different size from view. |
|
Today I will search in my code or try to reproduce the error, just wait a little |
|
Divider overrode: Divider from Material: This view is not using skeleton view |
There was a problem hiding this comment.
Thank you for your PR!
Overall the PR looks solid! However, there are some API changes that would potentially break existing apps. Also, as you said in #1252 (comment) calling layoutDivider() works. We can actually eliminate calling layoutDivider manually if we use autolayout as you suggested, but this PR does more than that. @adamdahan, @DanielDahan What do you think?
| case .bottom, .top: | ||
| superview.layout(self) | ||
| .leading(c.left, { _, _ in .equal }) | ||
| .trailing(c.right, {_, _ in .equal }) |
There was a problem hiding this comment.
See #1236 to learn about layout relations. You can reduce this to .leading(c.left)
|
|
||
| init(square: CGFloat) { | ||
| self.init(top: square, left: square, bottom: square, right: square) | ||
| } |
There was a problem hiding this comment.
I like these additions but would be better if you had done these changes in separate commits so that we could see changes separately with a nice commit message instead of sorting the changes in our mind.
There was a problem hiding this comment.
This is a nice little clean up.
|
|
||
| extension UIView { | ||
| /// TabBarItem reference. | ||
| public private(set) var divider: Divider { |
There was a problem hiding this comment.
It seems now instead of view.divider we need to do Divider.from(view: view). This is a radical API change
There was a problem hiding this comment.
I personally do not like the Divider.from(view: view) API change at all. Maybe we need to reconsider the entire divider on a UIView completely. @OrkhanAlikhanov @adamdahan
There was a problem hiding this comment.
Yup I agree with both of you @OrkhanAlikhanov @DanielDahan I like some of the refactoring done but overall the impact of this change is to large for existing users of Material.
|
@DanielDahan @OrkhanAlikhanov overall I think we should close this PR for now as it is a "one off situation" for @brennobemoura issue with his app. Let's utilize the idea's @brennobemoura presented for clean up and constraint based layout and figure out our overall position with Divider for our next big updates. |
|
@adamdahan I agree. This PR has presented some really nice ideas to consider, and I like that a lot, so thank you @brennobemoura. |
Updated Divider to be a UIView class using layout constraints. This approach turns divider more flexible to constraints updates from superview.