Skip to content

Conversation

alebo
Copy link
Contributor

@alebo alebo commented Oct 14, 2015

Trying to fix #127.

@woehrl01
Copy link
Contributor

@alebo: Great! this also seems to solve #83

But I had to pass: node.MaxWidth instead of just maxWidth

@woehrl01
Copy link
Contributor

@alebo: there is still an issue with this one. If you have MarginTop set. The result looks like that the value gets doubled somehow.

@alebo
Copy link
Contributor Author

alebo commented Oct 15, 2015

@woehrl01 Thanks for your comments. I can investigate it further.

@woehrl01
Copy link
Contributor

@alebo @vjeux I just looked at the tests, and in my opintion those are false negatives. If you add those children, the layout should be HasNewLayout, what do you think?

@alebo according your update, sorry to bother you but if you have a lot of nested streching layouts, sometimes the position gets fully set to 0 (they are left aligned), and so they are off. So I think this is still not totally correct :(

I think that reseting the mainAxisis enough:

child.layout[pos[mainAxis]] = 0;

at least in my scenarios the output is now as expected.

@alebo
Copy link
Contributor Author

alebo commented Oct 16, 2015

@woehrl01 if an item has MarginTop and MarginLeft resetting both axes might be required but it seems that only the margins (rather than the entire position) need to be reset.

Those failed tests seem to be false negatives indeed (but by pure accident they helped catch an actual problem too).
I'm going to push another update soon and I think I'll take the liberty to update the failed tests.

@alebo
Copy link
Contributor Author

alebo commented Oct 18, 2015

This also fixes #100

@woehrl01
Copy link
Contributor

@alebo looks really good to me. Awesome job! 👍

@woehrl01
Copy link
Contributor

@vjeux @lucasr any news on this one?

@corbt
Copy link

corbt commented Jan 20, 2016

What still needs to be done to get this merged? It has tests and (to my unfamiliar-with-css-layout eye) doesn't appear too complex.

@janmonschke
Copy link

@alebo looks good to me!

Would be great to get this merged :)

@alebo
Copy link
Contributor Author

alebo commented Jan 21, 2016

I'm ready to rebase this and resolve conflicts as soon as there's any feedback from the project maintainers.

@corbt
Copy link

corbt commented Jan 21, 2016

pinging @vjeux again, more people are running into this issue over at RN.

@vjeux
Copy link
Contributor

vjeux commented Jan 21, 2016

I'm sorry but won't be looking into it in a while. @lucasr do you have time to review this?

@lucasr
Copy link
Contributor

lucasr commented Jan 23, 2016

I think I need a little more context on the changes before reviewing (see questions).

@lucasr
Copy link
Contributor

lucasr commented Jan 23, 2016

And thanks for the contribution! ;-)

@senpng
Copy link

senpng commented Jan 27, 2016

image
image

This ''blue line" is not center;

when I set the parent view's height and the blue line is center

image
image

I think it is bug at here. because the blue line's parent view's height is determine.

jsendros added a commit to jsendros/css-layout that referenced this pull request Feb 21, 2016
…fined size in the cross-axis

Summary: Fetched and rebased
facebook#145 and updated it to do 2
things:
1) Make 'stretch' aligned children grow to fill their containers in the
cross-axis if the cross-axis size is undefined. The added test might be
the best explanation of this.
2) Make sure this doesn’t decrease the frequency of the
“simpleStackCross” optimization in a relatively complex sample
environment.

Test Plan: Added a test: Container with 3 row directioned children and
no explicit height set. One child is of height 0, one of 0 height with
stretch alignment, and one of 150 height. Expected the 0 height child
remains of height 0, stretch grows to 150, and 150 stays the same.
@andresn
Copy link

andresn commented Feb 22, 2016

@alebo @lucasr @vjeux any chance we can get some eyes on this again? I reduced the problem to a very simple example and there's a basic gap in meeting spec in RN:

Screenshot (expected behavior in RN's Fiddle layout sandbox, but not in iPhone simulator): http://screencast.com/t/H3Da1mQhmLX3
Fiddle: http://jsfiddle.net/drdrace/w95u54z6/1/

jsendros added a commit to jsendros/css-layout that referenced this pull request Feb 22, 2016
…fined size in the cross-axis

Summary: Fetched and rebased
facebook#145 and updated it to do 2
things:
1) Make 'stretch' aligned children grow to fill their containers in the
cross-axis if the cross-axis size is undefined. The added test might be
the best explanation of this.
2) Make sure this doesn’t decrease the frequency of the
“simpleStackCross” optimization in a relatively complex sample
environment.

Test Plan: Added a test: Container with 3 row directioned children and
no explicit height set. One child is of height 0, one of 0 height with
stretch alignment, and one of 150 height. Expected the 0 height child
remains of height 0, stretch grows to 150, and 150 stays the same.
@jsendros
Copy link

@andresn @alebo in #171 I built on top of the changes made here to fix another issue. None of the tests added/changed here break due to my changes in #171, so I expect this will fix your issue as well. :)

@alebo
Copy link
Contributor Author

alebo commented Feb 22, 2016

@lucasr, is there a requirement that a pull request must contain a single commit? Could @jsendros and myself create a collaborative PR with his changes and mine represented by separate commits (so that everyone can get their deserved praise and respect :)?

jsendros added a commit to jsendros/css-layout that referenced this pull request Feb 23, 2016
…fined size in the cross-axis

Summary: Fetched and rebased
facebook#145 and updated it to do 2
things:
1) Make 'stretch' aligned children grow to fill their containers in the
cross-axis if the cross-axis size is undefined. The added test might be
the best explanation of this.
2) Make sure this doesn’t decrease the frequency of the
“simpleStackCross” optimization in a relatively complex sample
environment.

Test Plan: Added a test: Container with 3 row directioned children and
no explicit height set. One child is of height 0, one of 0 height with
stretch alignment, and one of 150 height. Expected the 0 height child
remains of height 0, stretch grows to 150, and 150 stays the same.
jsendros added a commit to jsendros/css-layout that referenced this pull request Feb 23, 2016
…fined size in the cross-axis

Summary: Fetched and rebased
facebook#145 and updated it to do 2
things:
1) Make 'stretch' aligned children grow to fill their containers in the
cross-axis if the cross-axis size is undefined. The added test might be
the best explanation of this.
2) Make sure this doesn’t decrease the frequency of the
“simpleStackCross” optimization in a relatively complex sample
environment.

Test Plan: Added a test: Container with 3 row directioned children and
no explicit height set. One child is of height 0, one of 0 height with
stretch alignment, and one of 150 height. Expected the 0 height child
remains of height 0, stretch grows to 150, and 150 stays the same.
jsendros added a commit to jsendros/css-layout that referenced this pull request Feb 23, 2016
…fined size in the cross-axis

Summary: Fetched and rebased
facebook#145 and updated it to do 2
things:
1) Make 'stretch' aligned children grow to fill their containers in the
cross-axis if the cross-axis size is undefined. The added test might be
the best explanation of this.
2) Make sure this doesn’t decrease the frequency of the
“simpleStackCross” optimization in a relatively complex sample
environment.

Test Plan: Added a test: Container with 3 row directioned children and
no explicit height set. One child is of height 0, one of 0 height with
stretch alignment, and one of 150 height. Expected the 0 height child
remains of height 0, stretch grows to 150, and 150 stays the same.
jsendros added a commit to jsendros/css-layout that referenced this pull request Feb 23, 2016
…fined size in the cross-axis

Summary: Fetched and rebased
facebook#145 and updated it to do 2
things:
1) Make 'stretch' aligned children grow to fill their containers in the
cross-axis if the cross-axis size is undefined. The added test might be
the best explanation of this.
2) Make sure this doesn’t decrease the frequency of the
“simpleStackCross” optimization in a relatively complex sample
environment.

Test Plan: Added a test: Container with 3 row directioned children and
no explicit height set. One child is of height 0, one of 0 height with
stretch alignment, and one of 150 height. Expected the 0 height child
remains of height 0, stretch grows to 150, and 150 stays the same.
@lucasr
Copy link
Contributor

lucasr commented Feb 24, 2016

@alebo Could you please just push the exact diff from #171 here so that I can merge? @jsendros doesn't really mind having you as the author of this change.

alebo pushed a commit to alebo/css-layout that referenced this pull request Feb 24, 2016
…fined size in the cross-axis

Summary: Fetched and rebased
facebook#145 and updated it to do 2
things:
1) Make 'stretch' aligned children grow to fill their containers in the
cross-axis if the cross-axis size is undefined. The added test might be
the best explanation of this.
2) Make sure this doesn’t decrease the frequency of the
“simpleStackCross” optimization in a relatively complex sample
environment.

Test Plan: Added a test: Container with 3 row directioned children and
no explicit height set. One child is of height 0, one of 0 height with
stretch alignment, and one of 150 height. Expected the 0 height child
remains of height 0, stretch grows to 150, and 150 stays the same.
@alebo
Copy link
Contributor Author

alebo commented Feb 25, 2016

I've cherry-picked @jsendros's updates on top of my rebased commit. @lucasr, @jsendros, please confirm again, that you'd really like me to squash the two commits into one.

@jsendros
Copy link

Squash away!

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.

View not stretching vertically in column layout
9 participants