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

2015-05-06T00:22:46-07:00

prenaux commented

2015-05-06 00:22:46 -07:00

(Migrated from github.com)

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.

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.

vjeux commented

2015-05-07 08:56:46 -07:00

(Migrated from github.com)

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

vjeux commented

2015-05-07 09:01:15 -07:00

(Migrated from github.com)

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!

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 commented

2015-05-07 17:06:41 -07:00

(Migrated from github.com)

Yes, the additional test cases will be there shortly.

vjeux commented

2015-05-07 17:44:15 -07:00

(Migrated from github.com)

Awesome thanks :)

prenaux commented

2015-05-07 23:58:21 -07:00

(Migrated from github.com)

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.

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 commented

2015-05-08 00:20:24 -07:00

(Migrated from github.com)

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.

vjeux commented

2015-05-08 10:40:34 -07:00

(Migrated from github.com)

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

prenaux commented

2015-05-08 22:14:52 -07:00

(Migrated from github.com)

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

vjeux commented

2015-05-09 08:19:31 -07:00

(Migrated from github.com)

Thanks i'll take a look sometimes today!

prenaux commented

2015-05-09 09:23:29 -07:00

(Migrated from github.com)

Rebased on the latest version.

vjeux commented

2015-05-14 08:52:20 -07:00

(Migrated from github.com)

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 commented

2015-05-17 06:59:53 -07:00

(Migrated from github.com)

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

vjeux commented

2015-05-17 08:58:08 -07:00

(Migrated from github.com)

Thanks!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#79