Add YGLayoutGetMargin #335

Closed
arcanis wants to merge 4 commits from get-margin into master
arcanis commented 2017-01-12 01:57:03 -08:00 (Migrated from github.com)

Fix #326. I'll open another PR once this one gets accepted to add support for YGLayoutGetBorder 👌

Fix #326. I'll open another PR once this one gets accepted to add support for `YGLayoutGetBorder` 👌
emilsjolander commented 2017-01-12 02:15:29 -08:00 (Migrated from github.com)

Could you add the c# and java bindings for this as well? Should be fairly straight forward as it will look exactly like the padding ones.

Could you add the c# and java bindings for this as well? Should be fairly straight forward as it will look exactly like the padding ones.
arcanis commented 2017-01-12 02:19:58 -08:00 (Migrated from github.com)

Eh eh, I was working on them, they've just been pushed :) I haven't been able to test them yet, will Travis run the tests for those interfaces as well ?

Eh eh, I was working on them, they've just been pushed :) I haven't been able to test them yet, will Travis run the tests for those interfaces as well ?
emilsjolander commented 2017-01-12 02:22:47 -08:00 (Migrated from github.com)

Yup it will :) thanks!

Yup it will :) thanks!
arcanis commented 2017-01-12 03:10:17 -08:00 (Migrated from github.com)

Seems good, tests are now passing

Seems good, tests are now passing
arcanis commented 2017-01-12 03:46:24 -08:00 (Migrated from github.com)

@emilsjolander I started looking at how I can implement YGNodeLayoutGetBorder, and actually there will be a bit more work than I thought, since such a function would need to go through the same EDGE_START / EDGE_END check than its two siblings. Would you mind if I implemented it in this PR, since the code will be quite similar?

@emilsjolander I started looking at how I can implement `YGNodeLayoutGetBorder`, and actually there will be a bit more work than I thought, since such a function would need to go through the same `EDGE_START` / `EDGE_END` check than its two siblings. Would you mind if I implemented it in this PR, since the code will be quite similar?
emilsjolander commented 2017-01-12 03:49:21 -08:00 (Migrated from github.com)

@arcanis I would rather it was a PR based on top of this one. Trying to move to smaller pull requests where possible as it makes it much easier to grok the code. Thanks 👍

@arcanis I would rather it was a PR based on top of this one. Trying to move to smaller pull requests where possible as it makes it much easier to grok the code. Thanks 👍
arcanis commented 2017-01-12 03:49:57 -08:00 (Migrated from github.com)

Ok 👍

Ok 👍
emilsjolander (Migrated from github.com) reviewed 2017-01-12 05:28:31 -08:00
emilsjolander (Migrated from github.com) left a comment

Please add tests to YogaNodeTest.cs and YogaNodeTest.java. These will be similar to the tests already there for layoutPadding.

Please add tests to `YogaNodeTest.cs` and `YogaNodeTest.java`. These will be similar to the tests already there for layoutPadding.
emilsjolander (Migrated from github.com) commented 2017-01-12 05:25:29 -08:00

Don't add an Obsolete property :p all these will be removed soon enough.

Don't add an Obsolete property :p all these will be removed soon enough.
emilsjolander commented 2017-01-12 05:58:45 -08:00 (Migrated from github.com)

@arcanis Looks good now, thanks!

@arcanis Looks good now, thanks!
facebook-github-bot commented 2017-01-12 05:59:30 -08:00 (Migrated from github.com)

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

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

Pull request closed

Sign in to join this conversation.
No description provided.