[ASStackLayoutSpec] Flex wrap fix and lineSpacing property #472
Conversation
There was a problem hiding this comment.
Wow, this is awesome! You managed to fix a very well hidden bug and implement a frequently requested feature, all in a very compact and well-thought diff! The line spacing implementation, in particular, is very impressive. I don't recall it being defined/implemented anywhere else, including the original CSS flexbox spec, as well as its implementations such Yoga, servo's and WebKit's. While we'd like to stick to the CSS spec as much as possible, after reading your code, I'm convinced that it's a great addition to the framework that requires minimal maintenance and thus, is well-worth the divergence. In fact, I already have a use case for it in ASCollectionGalleryLayoutDelegate and ASCollectionFlowLayoutDelegate.
While it would be even better if we can have some new snapshot test cases for the bug fix and line spacing, I'm gonna approve this PR as is. Please feel free to follow up on the tests either in this PR or in a new one.
CHANGELOG.md
Outdated
| @@ -1,6 +1,7 @@ | |||
| ## master | |||
|
|
|||
| * Add your own contributions to the next release on the line below this with your name. | |||
| - [ASStackLayoutSpec] Fix flex wrap overflow and add lineSpacing property. [Flo Vouin](https://github.com/flovouin) | |||
There was a problem hiding this comment.
Since line spacing is a significant addition, I'd suggest to mention it in a separate line so people won't overlook it. It's your call though.
| @@ -70,6 +70,7 @@ + (ASCollectionLayoutState *)calculateLayoutWithContext:(ASCollectionLayoutConte | |||
| alignItems:ASStackLayoutAlignItemsStart | |||
| flexWrap:ASStackLayoutFlexWrapWrap | |||
| alignContent:ASStackLayoutAlignContentStart | |||
| lineSpacing:0 | |||
There was a problem hiding this comment.
Since we have a convenient initializer that doesn't require lineSpacing, please remove this change.
| @@ -81,6 +81,7 @@ + (ASCollectionLayoutState *)calculateLayoutWithContext:(ASCollectionLayoutConte | |||
| alignItems:ASStackLayoutAlignItemsStart | |||
| flexWrap:ASStackLayoutFlexWrapWrap | |||
| alignContent:ASStackLayoutAlignContentStart | |||
| lineSpacing:0 | |||
|
Hi Huy, thanks for the review! I do agree that tests would be a nice addition. I'll try to address this but unfortunately I can't give you a time estimate for this, so I think it would be best to go ahead with this PR and I'll submit another one in the future. Cheers, |
|
Ok, let's get this in. I created a separate issue for unit tests. Thanks for the awesome work, @flovouin! |
…oup#472) * ASStackUnpositionedLayout: Take spacing into account when laying out a wrapped stack. * ASStackLayoutSpec: Add the lineSpacing property. * Update CHANGELOG.md.
Hi guys,
I haven't opened an issue / feature request for this, but here it is, a PR that fixes a small bug in the flex wrap layout, and add a nice property to it.
lineSpacingproperty to theASStackLayoutSpecthat... well... adds space between lines. I tried to keep changes as minimal as possible, tell me what you think!As usual, here's a small demo project that demonstrates the bug and the new feature.
Cheers,
Flo