[Issue facebook/css-layout#127]: content re-layout of stretched items #145

2015-10-14T16:06:54-07:00

alebo commented

2015-10-14 16:06:54 -07:00

(Migrated from github.com)

Trying to fix #127.

woehrl01 commented

2015-10-15 04:57:54 -07:00

(Migrated from github.com)

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

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

@alebo: Great! this also seems to solve #83 [But I had to pass](https://github.com/facebook/css-layout/pull/145/files#diff-a69100b1f123b9ecff85330a582e3faaR932): `node.MaxWidth` instead of just `maxWidth`

woehrl01 commented

2015-10-15 05:36:01 -07:00

(Migrated from github.com)

@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: there is still an issue with this one. If you have `MarginTop` set. The result looks like that the value gets doubled somehow.

alebo commented

2015-10-15 08:39:49 -07:00

(Migrated from github.com)

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

woehrl01 commented

2015-10-16 01:36:15 -07:00

(Migrated from github.com)

@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 @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 `mainAxis`is enough: ``` child.layout[pos[mainAxis]] = 0; ``` at least in my scenarios the output is now as expected.

alebo commented

2015-10-16 04:08:13 -07:00

(Migrated from github.com)

@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.

@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 commented

2015-10-18 04:03:59 -07:00

(Migrated from github.com)

This also fixes #100

woehrl01 commented

2015-10-18 23:43:16 -07:00

(Migrated from github.com)

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

@alebo looks really good to me. Awesome job! :+1:

woehrl01 commented

2015-12-15 02:56:49 -08:00

(Migrated from github.com)

@vjeux @lucasr any news on this one?

corbt commented

2016-01-20 06:02:05 -08:00

(Migrated from github.com)

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 commented

2016-01-21 05:33:51 -08:00

(Migrated from github.com)

@alebo looks good to me!

Would be great to get this merged :)

@alebo looks good to me! Would be great to get this merged :)

alebo commented

2016-01-21 12:41:02 -08:00

(Migrated from github.com)

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

corbt commented

2016-01-21 12:44:36 -08:00

(Migrated from github.com)

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

vjeux commented

2016-01-21 13:11:34 -08:00

(Migrated from github.com)

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

lucasr commented

2016-01-23 02:38:34 -08:00

(Migrated from github.com)

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

lucasr commented

2016-01-23 02:38:45 -08:00

(Migrated from github.com)

And thanks for the contribution! ;-)

senpng commented

2016-01-26 22:49:06 -08:00

(Migrated from github.com)

This ''blue line" is not center;

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

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

![image](https://cloud.githubusercontent.com/assets/7333114/12606012/fb8388e2-c504-11e5-8413-5810f364258b.png) ![image](https://cloud.githubusercontent.com/assets/7333114/12606017/007ed31a-c505-11e5-962e-1d8f16650e1f.png) This ''blue line" is not center; when I set the parent view's height and the blue line is center ![image](https://cloud.githubusercontent.com/assets/7333114/12606027/10088e5c-c505-11e5-9767-6a0eb67e9ac6.png) ![image](https://cloud.githubusercontent.com/assets/7333114/12606031/168c54a2-c505-11e5-9bc7-7b307e53555b.png) I think it is bug at here. because the blue line's parent view's height is determine.

andresn commented

2016-02-22 09:02:26 -08:00

(Migrated from github.com)

@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/

@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 commented

2016-02-22 09:19:58 -08:00

(Migrated from github.com)

@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 commented

2016-02-22 12:18:45 -08:00

(Migrated from github.com)

@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 :)?

lucasr commented

2016-02-24 07:48:27 -08:00

(Migrated from github.com)

@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 commented

2016-02-25 03:14:46 -08:00

(Migrated from github.com)

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 commented

2016-02-25 10:47:24 -08:00

(Migrated from github.com)

Squash away!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#145