Measure nodes which have margin: auto and align-item: stretch #645

Closed
woehrl01 wants to merge 2 commits from margin_auto_measure into master
woehrl01 commented 2017-10-08 10:36:45 -07:00 (Migrated from github.com)

If you have a measurable node and set marign-left: auto + align-item:stretch on it, it won't get measured and they get a width/height of -(nan). This change fixes that behaviour. Fixes #644.

If you have a measurable node and set ```marign-left: auto``` + ```align-item:stretch``` on it, it won't get measured and they get a width/height of ```-(nan)```. This change fixes that behaviour. Fixes #644.
emilsjolander commented 2017-11-24 06:26:36 -08:00 (Migrated from github.com)

@woehrl01 Can you ensure we have a test case to catch the case where we have auto + stretch without a custom measure? Both when node has explicit and non-explcit size set.

@woehrl01 Can you ensure we have a test case to catch the case where we have auto + stretch without a custom measure? Both when node has explicit and non-explcit size set.
woehrl01 commented 2017-11-24 06:34:45 -08:00 (Migrated from github.com)

If I remember correctly we already have that in place for non custom measure nodes. This change is only needed for custom measure nodes. I added those when I initially added the auto margin support.

If I remember correctly we already have that in place for non custom measure nodes. This change is only needed for custom measure nodes. I added those when I initially added the auto margin support.
emilsjolander commented 2017-11-24 06:37:08 -08:00 (Migrated from github.com)

I can't find any test in https://github.com/facebook/yoga/blob/master/tests/YGMarginTest.cpp which sets auto margin without defined width/height (thus relying on stretch behaviour). Could you add one?

I can't find any test in https://github.com/facebook/yoga/blob/master/tests/YGMarginTest.cpp which sets auto margin without defined width/height (thus relying on stretch behaviour). Could you add one?
woehrl01 commented 2017-11-24 06:38:24 -08:00 (Migrated from github.com)

Sure 👍 I'll have a look in the next couple of hours. I'm currently out of office.

Sure 👍 I'll have a look in the next couple of hours. I'm currently out of office.
emilsjolander commented 2017-11-24 06:39:43 -08:00 (Migrated from github.com)

👍

👍
woehrl01 commented 2017-11-24 06:56:59 -08:00 (Migrated from github.com)

@emilsjolander I hope these are the kind of tests you are looking for. 🔬

@emilsjolander I hope these are the kind of tests you are looking for. 🔬
emilsjolander commented 2017-11-27 04:04:46 -08:00 (Migrated from github.com)

Looks good!

Looks good!
facebook-github-bot (Migrated from github.com) reviewed 2017-11-27 04:05:15 -08:00
facebook-github-bot (Migrated from github.com) left a comment

@emilsjolander is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@emilsjolander is landing this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D6413512).

Pull request closed

Sign in to join this conversation.
No description provided.