Skip to content
This repository was archived by the owner on Feb 2, 2023. It is now read-only.

Conversation

rcancro
Copy link
Contributor

@rcancro rcancro commented Oct 2, 2015

I noticed when creating a static layout that the sizeRange of the children was being ignored. The case was:

I had an image as a child of a static layout
The image was set to have an exact range of 90x90
When the static layout was measured, the constrained size came in as 375xInf (the width of the containing node and unbounded)
The static layout computed its final size as 375x90

According to the comments in component kit's header file the static layout "[c]omputes a size that is the union of all childrens' frames." It appeared that we are only doing that in the unbounded direction. My fix is to do it in both directions.

@nguyenhuy Please take a look and let me know what you think.

I noticed when creating a static layout that the sizeRange of the children was being ignored. The case was:

I had an image as a child of a static layout
The image was set to have an exact range of 90x90
When the static layout was measured, the constrained size came in as 375xInf (the width of the containing node and unbounded)
The static layout computed its final size as 375x90

According to the comments in component kit's header file the static layout  "[c]omputes a size that is the union of all childrens' frames." It appeared that we are only doing that in the unbounded direction. My fix is to do it in both directions.
@appleguy
Copy link
Contributor

appleguy commented Oct 3, 2015

Thanks for finding this! Definitely interested in Huy's thoughts as well.

@nguyenhuy
Copy link
Contributor

nguyenhuy commented Oct 3, 2015

Thanks for bringing this up. I agree that this diff does make the spec works accordingly to its description. So it looks good to me.

The current wrap-content behaviour is good and should work for most use cases. In addition to that, we may add a flag that indicates how the spec should determine its size, either by wrapping its content or filling its parent (for each direction). Such flag already exists in center spec, but let's do this once there is a demand, I guess.

@appleguy
Copy link
Contributor

appleguy commented Oct 3, 2015

@rcancro, @nguyenhuy eager to merge this — but could you take a half hour or so to devise a test that fails before the fix, and passes after? Maybe a snapshot test that evaluates against the colored rectangle images?

@nguyenhuy
Copy link
Contributor

Not sure if I can push to rcancro:staticLayoutbranch. But I will write a bunch of tests for ASStaticLayoutSpec in the coming days.

appleguy added a commit that referenced this pull request Oct 4, 2015
Fix static layout to ensure it always considers its sub layout's preferred sizes.
@appleguy appleguy merged commit 057422d into facebookarchive:master Oct 4, 2015
@appleguy
Copy link
Contributor

appleguy commented Oct 4, 2015

Thanks a lot, @nguyenhuy! I'll merge this now, and feel free to put a test into any future commit. As long as you're tracking it, it isn't an urgent need, just didn't want it to be forgotten.

peter-iakovlev pushed a commit to peter-iakovlev/AsyncDisplayKit that referenced this pull request Jan 9, 2018
…acebookarchive#699)

* Revert "Adds support for specifying a quality indexed array of URLs (facebookarchive#557)"

This reverts commit 3c77d4a.

* Add CHANGELOG entry
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants