[Issue facebook/css-layout#78]: Implemented alignContent ; #79

Merged
merged 15 commits into from May 17, 2015

Conversation

Projects
None yet
2 participants
@prenaux
Contributor

prenaux commented May 6, 2015

So this implements align-content. It follows the W3C specs, that's how it works in Chrome as far as I have tested. I tried to make sure that it didn't touch any pre-existing code as much as possible.

All the current tests are working (in RunLayoutTests.html and with make - JS, C & Java). RunLayoutRandomTests.html is failing but then it was failing without these changes so I suppose its an issue with the random test suite not being up-to-date.

I'm going to add some more test cases in Jasmine and add this in react-native. From simple tests it works fine in react-native but I need to add alignContent in there to make sure that the new feature works correctly.

Let me know if you have any comment about the implementation itself.

@prenaux prenaux referenced this pull request in facebook/react-native May 7, 2015

Closed

Support for alignContent in the StyleSheet & Layout ; #1171

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux May 7, 2015

Contributor

Thanks a lot for working on this! I'm going to take a look at it shortly :)

Contributor

vjeux commented May 7, 2015

Thanks a lot for working on this! I'm going to take a look at it shortly :)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux May 7, 2015

Contributor

Right now random tests are broken for some really edgy cases. What I usually do is to disable the attributes that are making those tests to fail until they are clean. Then, add your new feature and run the automated tests.

I cannot really bring this in without any tests, can you make sure to add at least 5 test cases to make sure that your feature is working correctly, but more importantly that if we make further modifications down the road, it won't break down.

Thanks! This is super exciting!

Contributor

vjeux commented May 7, 2015

Right now random tests are broken for some really edgy cases. What I usually do is to disable the attributes that are making those tests to fail until they are clean. Then, add your new feature and run the automated tests.

I cannot really bring this in without any tests, can you make sure to add at least 5 test cases to make sure that your feature is working correctly, but more importantly that if we make further modifications down the road, it won't break down.

Thanks! This is super exciting!

@prenaux

This comment has been minimized.

Show comment
Hide comment
@prenaux

prenaux May 8, 2015

Contributor

Yes, the additional test cases will be there shortly.

Contributor

prenaux commented May 8, 2015

Yes, the additional test cases will be there shortly.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux May 8, 2015

Contributor

Awesome thanks :)

Contributor

vjeux commented May 8, 2015

Awesome thanks :)

@prenaux

This comment has been minimized.

Show comment
Hide comment
@prenaux

prenaux May 8, 2015

Contributor

I've added 16 test cases for the new alignContent code.

I also added pixel snapping in the layout tests, please check the code for a more complete description, but the issue is that chrome seems to change its layout code so the floating points values of layoutDom change from version to version, the snapping code I added assumes that it's ok to be 'only' pixel perfect, and that we don't care if there's a sub-pixel difference.

I have Chrome 42.0.2311.135 (64-bit) on OSX and 19 tests fail (without the alignContent changes - just after pulling the current master) because of floating point deviations. The biggest issue imo is that my Windows machine still works fine, so even if the code is 'fixed' for Chrome 42 on OSX other platforms might still give different results.

Contributor

prenaux commented May 8, 2015

I've added 16 test cases for the new alignContent code.

I also added pixel snapping in the layout tests, please check the code for a more complete description, but the issue is that chrome seems to change its layout code so the floating points values of layoutDom change from version to version, the snapping code I added assumes that it's ok to be 'only' pixel perfect, and that we don't care if there's a sub-pixel difference.

I have Chrome 42.0.2311.135 (64-bit) on OSX and 19 tests fail (without the alignContent changes - just after pulling the current master) because of floating point deviations. The biggest issue imo is that my Windows machine still works fine, so even if the code is 'fixed' for Chrome 42 on OSX other platforms might still give different results.

@prenaux

This comment has been minimized.

Show comment
Hide comment
@prenaux

prenaux May 8, 2015

Contributor

Added alignContent in the random tests permutations, and also added pixel snapping in the random tests - so that sub-pixel errors are not counted as 'random' errors.

Contributor

prenaux commented May 8, 2015

Added alignContent in the random tests permutations, and also added pixel snapping in the random tests - so that sub-pixel errors are not counted as 'random' errors.

src/Layout-test-utils.js
+ var val = aObj[key];
+ switch (typeof(val)) {
+ case 'number': {
+ aObj[key] = Math.floor((val*testMeasurePrecision)+0.5)/testMeasurePrecision;

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: can you put space around *, + and /

@vjeux

vjeux May 8, 2015

Contributor

nit: can you put space around *, + and /

src/Layout-test-utils.js
+ if (!aObj.hasOwnProperty(key))
+ continue;
+ var val = aObj[key];
+ switch (typeof(val)) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: typeof val

@vjeux

vjeux May 8, 2015

Contributor

nit: typeof val

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

Can you use an if/then/else, it makes the code shorter:

if (typeof val === 'number') {
  obj[key] = ...;
} else if (typeof val === 'object') {
  inplaceRondNumbersInObject(val);
}
@vjeux

vjeux May 8, 2015

Contributor

Can you use an if/then/else, it makes the code shorter:

if (typeof val === 'number') {
  obj[key] = ...;
} else if (typeof val === 'object') {
  inplaceRondNumbersInObject(val);
}
src/Layout-test-utils.js
+ return;
+
+ for (var key in aObj) {
+ if (!aObj.hasOwnProperty(key))

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: please always add {}

@vjeux

vjeux May 8, 2015

Contributor

nit: please always add {}

src/Layout-test-utils.js
@@ -227,6 +240,27 @@ var layoutTestUtils = (function() {
return layout;
}
+ function inplaceRoundNumbersInObject(aObj) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

Can you name it obj

@vjeux

vjeux May 8, 2015

Contributor

Can you name it obj

src/Layout-test-utils.js
@@ -227,6 +240,27 @@ var layoutTestUtils = (function() {
return layout;
}
+ function inplaceRoundNumbersInObject(aObj) {
+ if (!testMeasurePrecision) // undefined/0, disables rounding

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

braces

@vjeux

vjeux May 8, 2015

Contributor

braces

src/Layout-test-utils.js
@@ -337,11 +371,13 @@ var layoutTestUtils = (function() {
div.style.display = 'flex';
div.style.flexDirection = 'column';
div.style.alignItems = 'flex-start';
+ div.style.alignContent = 'stretch';

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

didn't you say that we currently have flex-start as the default?

@vjeux

vjeux May 8, 2015

Contributor

didn't you say that we currently have flex-start as the default?

This comment has been minimized.

@prenaux

prenaux May 9, 2015

Contributor

'flex-start' is similar to the previous react-native 'result', 'stretch' is the default in standard CSS so I left it at that.

@prenaux

prenaux May 9, 2015

Contributor

'flex-start' is similar to the previous react-native 'result', 'stretch' is the default in standard CSS so I left it at that.

This comment has been minimized.

@vjeux

vjeux May 9, 2015

Contributor

I found flexbox defaults to be pretty terrible, the ones that are in React native work really well to build apps. Also, when I import this back to fb codebase, I don't want everything to break under me!

@vjeux

vjeux May 9, 2015

Contributor

I found flexbox defaults to be pretty terrible, the ones that are in React native work really well to build apps. Also, when I import this back to fb codebase, I don't want everything to break under me!

This comment has been minimized.

@prenaux

prenaux May 9, 2015

Contributor

Ok, fine by me :) I'll change it to flex-start.

@prenaux

prenaux May 9, 2015

Contributor

Ok, fine by me :) I'll change it to flex-start.

src/Layout.c
startLine = endLine;
}
+ // <Loop DD>
+ //
+ // PIERRE: More than one line, we need to layout the crossAxis according to

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

Can you use the following formatting:

// Note(prenaux): 
@vjeux

vjeux May 8, 2015

Contributor

Can you use the following formatting:

// Note(prenaux): 
src/Layout.c
+ }
+ else if (alignItem == CSS_ALIGN_STRETCH) {
+ crossPosition = currentLead + getMargin(child,leading[crossAxis]);
+ // TODO: Correctly set the height of items with undefined (auto)

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

TODO(prenaux)

@vjeux

vjeux May 8, 2015

Contributor

TODO(prenaux)

src/Layout.c
startLine = endLine;
}
+ // <Loop DD>

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

Loop E?

@vjeux

vjeux May 8, 2015

Contributor

Loop E?

This comment has been minimized.

@prenaux

prenaux May 9, 2015

Contributor

There's loop E below (line 794), I can rename that to F, and put this to E.

@prenaux

prenaux May 9, 2015

Contributor

There's loop E below (line 794), I can rename that to F, and put this to E.

src/Layout.c
+ }
+ else if (alignContent == CSS_ALIGN_STRETCH) {
+ if (nodeCrossAxisInnerSize > linesCrossDim) {
+ crossDimAdd = (remainingCrossDim / linesCount);

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: no need for parenthesis

@vjeux

vjeux May 8, 2015

Contributor

nit: no need for parenthesis

src/Layout.c
+ int lineIndex = -1;
+
+ // get the first child on the current line
+ {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

why do you create a new block?

@vjeux

vjeux May 8, 2015

Contributor

why do you create a new block?

This comment has been minimized.

@prenaux

prenaux May 9, 2015

Contributor

I was declaring another variable in there, I switched to use the 'child' variable when I converted the code to JS/transpiler. Removed it.

@prenaux

prenaux May 9, 2015

Contributor

I was declaring another variable in there, I switched to use the 'child' variable when I converted the code to JS/transpiler. Removed it.

src/Layout.c
+ if (alignContent == CSS_ALIGN_FLEX_END) {
+ currentLead += remainingCrossDim;
+ }
+ else if (alignContent == CSS_ALIGN_CENTER) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: } and else if on the same line

@vjeux

vjeux May 8, 2015

Contributor

nit: } and else if on the same line

src/Layout.c
+ }
+ else if (alignContent == CSS_ALIGN_CENTER) {
+ currentLead += remainingCrossDim / 2;
+ }

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

same here

@vjeux

vjeux May 8, 2015

Contributor

same here

src/Layout.c
+ break;
+ }
+ if (!isUndefined(child->layout.dimensions[dim[crossAxis]])) {
+ lineHeight = fmaxf(lineHeight,child->layout.dimensions[dim[crossAxis]] +

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: indent like this:

lineHeight = fmaxf(
  lineHeight,
  child->layout.dimensions[dim[crossAxis]] + getMarginAxis(child,crossAxis)
);
@vjeux

vjeux May 8, 2015

Contributor

nit: indent like this:

lineHeight = fmaxf(
  lineHeight,
  child->layout.dimensions[dim[crossAxis]] + getMarginAxis(child,crossAxis)
);
src/Layout.c
+ if (alignItem == CSS_ALIGN_FLEX_START) {
+ crossPosition = currentLead + getMargin(child,leading[crossAxis]);
+ }
+ else if (alignItem == CSS_ALIGN_FLEX_END) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

same line

@vjeux

vjeux May 8, 2015

Contributor

same line

src/Layout.c
+ getMargin(child,trailing[crossAxis]) -
+ child->layout.dimensions[dim[crossAxis]];
+ }
+ else if (alignItem == CSS_ALIGN_CENTER) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

same line

@vjeux

vjeux May 8, 2015

Contributor

same line

src/Layout.c
+ float childHeight = child->layout.dimensions[dim[crossAxis]];
+ crossPosition = currentLead + ((lineHeight - childHeight)/2);
+ }
+ else if (alignItem == CSS_ALIGN_STRETCH) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

same line

@vjeux

vjeux May 8, 2015

Contributor

same line

src/Layout.c
+ crossPosition = currentLead + ((lineHeight - childHeight)/2);
+ }
+ else if (alignItem == CSS_ALIGN_STRETCH) {
+ crossPosition = currentLead + getMargin(child,leading[crossAxis]);

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: space after ,

@vjeux

vjeux May 8, 2015

Contributor

nit: space after ,

src/Layout.c
+ css_align_t alignItem = getAlignItem(node, child);
+ float crossPosition = child->layout.position[pos[crossAxis]]; // preserve current position if someting goes wrong with alignItem?
+ if (alignItem == CSS_ALIGN_FLEX_START) {
+ crossPosition = currentLead + getMargin(child,leading[crossAxis]);

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: space after ,

@vjeux

vjeux May 8, 2015

Contributor

nit: space after ,

src/__tests__/Layout-test.js
@@ -1586,5 +1587,78 @@ describe('Layout', function() {
);
});
+});
+
+describe('Layout alignContent', function() {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

Can you figure out why those tests are not being transpiled to C and Java? Is it because you are creating a new describe?

@vjeux

vjeux May 8, 2015

Contributor

Can you figure out why those tests are not being transpiled to C and Java? Is it because you are creating a new describe?

This comment has been minimized.

@prenaux

prenaux May 9, 2015

Contributor

Most likely, going to check that, I didn't know they were supposed to be transpiled to C ;)

@prenaux

prenaux May 9, 2015

Contributor

Most likely, going to check that, I didn't know they were supposed to be transpiled to C ;)

src/__tests__/Layout-test.js
+ var alignContentData = {
+ style: {width: 300, height: 380, flexDirection: 'row', flexWrap: 'wrap'},
+ children: [
+ /* 0 */ { style: {width: 50, height: 50, margin: 10} },

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: no space right after { and before the }

@vjeux

vjeux May 8, 2015

Contributor

nit: no space right after { and before the }

src/__tests__/Layout-test.js
+ testLayout(
+ alignContentData,
+ { width:300,height:380,top:0,left:0, children:[
+ {width:50,height:50,top:10,left:10},

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: space after : and ,

@vjeux

vjeux May 8, 2015

Contributor

nit: space after : and ,

src/__tests__/Layout-test.js
+ it('should layout with alignContent: stretch, and alignItems: flex-start', function() {
+ testLayout(
+ alignContentData,
+ { width:300,height:380,top:0,left:0, children:[

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: no space after { but space before [

@vjeux

vjeux May 8, 2015

Contributor

nit: no space after { but space before [

src/__tests__/Layout-test.js
+ });
+
+ function testAlignContent(aAlignContent, aAlignItems) {
+ it('should layout with alignContent: '+aAlignContent+', and alignItems: '+aAlignItems, function() {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: space around +

@vjeux

vjeux May 8, 2015

Contributor

nit: space around +

src/__tests__/Layout-test.js
+ });
+ }
+
+ // testAlignContent('stretch', 'flex-start'); // above with expected value data

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

why is it commented out?

@vjeux

vjeux May 8, 2015

Contributor

why is it commented out?

This comment has been minimized.

@prenaux

prenaux May 9, 2015

Contributor

Its the same as 'should layout with alignContent: stretch, and alignItems: flex-start'. Difference is that those extra 15 tests are only against the DOM, they don't have a set of expected data in the code... mostly because I didn't really want to copy/paste the data 15 times from Chrome. I didn't know this was transpiled to C eventually, probably going to have to update it ;)

@prenaux

prenaux May 9, 2015

Contributor

Its the same as 'should layout with alignContent: stretch, and alignItems: flex-start'. Difference is that those extra 15 tests are only against the DOM, they don't have a set of expected data in the code... mostly because I didn't really want to copy/paste the data 15 times from Chrome. I didn't know this was transpiled to C eventually, probably going to have to update it ;)

src/__tests__/Layout-test.js
+ );
+ });
+
+ function testAlignContent(aAlignContent, aAlignItems) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: remove a at the front of the variable name

@vjeux

vjeux May 8, 2015

Contributor

nit: remove a at the front of the variable name

src/Layout.js
+ }
+
+ // find the first node on the first line
+ for (i = 0; i < node.children.length; ) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

this loop is super scary, it is hard to know if it's going to terminate by reading the code. Is there any way you can make it a standard loop over the array?

@vjeux

vjeux May 8, 2015

Contributor

this loop is super scary, it is hard to know if it's going to terminate by reading the code. Is there any way you can make it a standard loop over the array?

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

what about for

for (lineIndex = 0; lineIndex < linesCount; ++lineIndex) {
@vjeux

vjeux May 8, 2015

Contributor

what about for

for (lineIndex = 0; lineIndex < linesCount; ++lineIndex) {
src/Layout.js
+ }
+
+ var/*css_align_t*/ alignItem = getAlignItem(node, child);
+ var/*float*/ crossPosition = child.layout[pos[crossAxis]]; // preserve current position if someting goes wrong with alignItem?

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

you don't need this since you handle all the cases

@vjeux

vjeux May 8, 2015

Contributor

you don't need this since you handle all the cases

This comment has been minimized.

@prenaux

prenaux May 9, 2015

Contributor

My defensive reflexes from C, its in case someone adds another possible value for alignItem that I'm not handling. Anyway got removed since I assign to child.layout[pos[crossAxis]] directly in the end.

@prenaux

prenaux May 9, 2015

Contributor

My defensive reflexes from C, its in case someone adds another possible value for alignItem that I'm not handling. Anyway got removed since I assign to child.layout[pos[crossAxis]] directly in the end.

src/Layout.c
+ }
+ else if (alignItem == CSS_ALIGN_CENTER) {
+ float childHeight = child->layout.dimensions[dim[crossAxis]];
+ crossPosition = currentLead + ((lineHeight - childHeight)/2);

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: you don't need parenthesis around (a / 2) because / has precedence over +
nit: space around /

@vjeux

vjeux May 8, 2015

Contributor

nit: you don't need parenthesis around (a / 2) because / has precedence over +
nit: space around /

src/Layout.c
+ //
+ if (linesCount > 1 &&
+ (!isUndefined(node->layout.dimensions[dim[crossAxis]])))
+ {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

nit: don't need parenthesis around !isUndefined and put { on the same line

if (linesCount > 1 &&
    !isUndefined(node->layout.dimensions[dim[crossAxis]])) {
@vjeux

vjeux May 8, 2015

Contributor

nit: don't need parenthesis around !isUndefined and put { on the same line

if (linesCount > 1 &&
    !isUndefined(node->layout.dimensions[dim[crossAxis]])) {
src/Layout.js
+ if (child.lineIndex != lineIndex) {
+ break;
+ }
+ if (!isUndefined(child.layout[dim[crossAxis]])) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

what if none of the children have a specified dimension? Is it properly handled?

@vjeux

vjeux May 8, 2015

Contributor

what if none of the children have a specified dimension? Is it properly handled?

src/Layout.js
+ var/*int*/ startIndex = i;
+ var/*int*/ lineIndex = -1;
+
+ // get the first child on the current line

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

if you use my suggestion above, you can just get rid of this block to find the lineIndex since you have it

@vjeux

vjeux May 8, 2015

Contributor

if you use my suggestion above, you can just get rid of this block to find the lineIndex since you have it

src/Layout.js
+ }
+ }
+ var/*int*/ endIndex = ii;
+ lineHeight += crossDimAdd;

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

can you make crossDimAdd variable name more descriptive?

@vjeux

vjeux May 8, 2015

Contributor

can you make crossDimAdd variable name more descriptive?

src/Layout.js
+ // PIERRE: More than one line, we need to layout the crossAxis according to
+ // alignContent.
+ //
+ // Note that we could probably remove <Loop D> and handle the one line case

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

I think it's good to have it as a separate loop

@vjeux

vjeux May 8, 2015

Contributor

I think it's good to have it as a separate loop

src/Layout.js
+ var/*float*/ lineHeight = 0;
+ for (ii = startIndex; ii < node.children.length; ++ii) {
+ child = node.children[ii];
+ if (getPositionType(child) != CSS_POSITION_RELATIVE) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

please use !==

@vjeux

vjeux May 8, 2015

Contributor

please use !==

src/Layout.js
+ if (getPositionType(child) != CSS_POSITION_RELATIVE) {
+ continue;
+ }
+ if (child.lineIndex != lineIndex) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

!==

@vjeux

vjeux May 8, 2015

Contributor

!==

src/Layout.js
+
+ for (ii = startIndex; ii < endIndex; ++ii) {
+ child = node.children[ii];
+ if (getPositionType(child) != CSS_POSITION_RELATIVE) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

!==

@vjeux

vjeux May 8, 2015

Contributor

!==

src/Layout.js
+ if (alignItem == CSS_ALIGN_FLEX_START) {
+ crossPosition = currentLead + getMargin(child,leading[crossAxis]);
+ }
+ else if (alignItem == CSS_ALIGN_FLEX_END) {

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor
===

(same for all the == in the js file)

@vjeux

vjeux May 8, 2015

Contributor
===

(same for all the == in the js file)

src/Layout.js
+ // TODO: Correctly set the height of items with undefined (auto)
+ // crossAxis dimension.
+ }
+ child.layout[pos[crossAxis]] = crossPosition;

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

since you always assign to child.layout[pos[crossAxis]], do you really need the variable? Can you just assign it in every branch of the if?

@vjeux

vjeux May 8, 2015

Contributor

since you always assign to child.layout[pos[crossAxis]], do you really need the variable? Can you just assign it in every branch of the if?

This comment has been minimized.

@prenaux

prenaux May 9, 2015

Contributor

yup

@prenaux

prenaux May 9, 2015

Contributor

yup

@@ -65,6 +65,7 @@ describe('Random layout', function() {
randEnum(node, 0.5, 'justifyContent', ['flex-start', 'center', 'flex-end', 'space-between', 'space-around']);
randEnum(node, 0.5, 'alignItems', ['flex-start', 'center', 'flex-end', 'stretch']);
randEnum(node, 0.5, 'alignSelf', ['flex-start', 'center', 'flex-end', 'stretch']);
+ randEnum(node, 0.5, 'alignContent', ['flex-start', 'center', 'flex-end', 'stretch']);

This comment has been minimized.

@vjeux

vjeux May 8, 2015

Contributor

👍

@vjeux

vjeux May 8, 2015

Contributor

👍

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux May 8, 2015

Contributor

Thanks! I added a lot of stylistic comments, but overall the code looks good. I'm excited to ship it :)

Contributor

vjeux commented May 8, 2015

Thanks! I added a lot of stylistic comments, but overall the code looks good. I'm excited to ship it :)

@prenaux

This comment has been minimized.

Show comment
Hide comment
@prenaux

prenaux May 9, 2015

Contributor

I think all has been styled / fixed, let me know if I missed anything :)

Contributor

prenaux commented May 9, 2015

I think all has been styled / fixed, let me know if I missed anything :)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux May 9, 2015

Contributor

Thanks i'll take a look sometimes today!

Contributor

vjeux commented May 9, 2015

Thanks i'll take a look sometimes today!

prenaux added some commits May 6, 2015

[src/Layout-test-utils.js]: Added testMeasurePrecision and inplaceRou…
…ndNumbersInObject which snap the numbers to pixels so that we don't have to re-adjust for each browser version (Chrome 42 changed their computation, it can output floating point values for dom elements) ;
[src/__tests__/Layout-test.js]: Added 16 test cases for each alignCon…
…tent / alignItems combination - also includes alignSelf testing within the test data ;
[src/Layout-test-utils.js]: Added inplaceRoundNumbersInObject to test…
…RandomLayout so that much less random tests fail ; Moved inplaceRoundNumbersInObject in the main functions instead of having it in nameLayout ;
[Makefile]: Updated so that it works on OSX & Windows - for Windows i…
…t requires GCC, make 3.82 & wget to be on PATH ;
Made relevant JS tests transpile to C ; [src/Layout.c]: print_css_nod…
…e_rec(): print alignContent ; [src/Layout-test-utils.c]: add_failed_test(): Sets failed_test->next to NULL, otherwise the test crashes if there's one and only one failure ; Added type casts so that it can be compiled as C++ by MSVC on Windows ; [Makefile]: Added c_test_msvc target when running in Windows so that the test executable can be built and debugged with Visual Studio on Windows ;
[src/Layout.c & Layout-test-utils.c]: Don't define fmaxf/fminf if we'…
…re building with VC12+, its already declared from then on ;
@prenaux

This comment has been minimized.

Show comment
Hide comment
@prenaux

prenaux May 9, 2015

Contributor

Rebased on the latest version.

Contributor

prenaux commented May 9, 2015

Rebased on the latest version.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux May 14, 2015

Contributor

Ergg, I'm sorry but can you rebase, it is no longer merging cleanly. Sorry for the long delay, I marked the mail as read and forgot about it, sorry :(

Contributor

vjeux commented May 14, 2015

Ergg, I'm sorry but can you rebase, it is no longer merging cleanly. Sorry for the long delay, I marked the mail as read and forgot about it, sorry :(

@prenaux

This comment has been minimized.

Show comment
Hide comment
@prenaux

prenaux May 17, 2015

Contributor

Ok, merged. I re-runned the automated tests and the tests I have in React native ;

Contributor

prenaux commented May 17, 2015

Ok, merged. I re-runned the automated tests and the tests I have in React native ;

vjeux added a commit that referenced this pull request May 17, 2015

Merge pull request #79 from prenaux/master
[Issue facebook/yoga#78]: Implemented alignContent ;

@vjeux vjeux merged commit ee1cbac into facebook:master May 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux May 17, 2015

Contributor

Thanks!

Contributor

vjeux commented May 17, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment