DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Added property display: flex and none #369

Closed
woehrl01 wants to merge 6 commits from feature-hidden-nodes into master
pull from: feature-hidden-nodes
merge into: DaddyFrosty:master
Conversation 15 Commits 6 Files Changed 26 +1936 -418
woehrl01 commented 2017-01-31 11:46:36 -08:00 (Migrated from github.com)
Copy Link

Fix #241 and successor for #302

Added new property display with YGDisplayFlex and YGDisplayNone. Allows to hide nodes from the layout without the need to remove it from the DOM.

Fix #241 and successor for #302 Added new property ```display``` with ```YGDisplayFlex``` and ```YGDisplayNone```. Allows to hide nodes from the layout without the need to remove it from the DOM.
emilsjolander commented 2017-01-31 12:11:45 -08:00 (Migrated from github.com)
Copy Link

@joce does this solve it in a way that would work for you?
@woehrl01 could you add API bindings for java/csharp/objc/js?

I'll come back tomorrow and comment on the implementation. API looks good though. Thanks!

@joce does this solve it in a way that would work for you? @woehrl01 could you add API bindings for java/csharp/objc/js? I'll come back tomorrow and comment on the implementation. API looks good though. Thanks!
woehrl01 commented 2017-01-31 12:21:14 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander I could give it a try, but I only have c++ set up at the moment (new laptop) so I have to rely on travis, will take me "some" time. Not sure about js/nbind, maybe @arcanis could do a follow up 😊 please?

@emilsjolander I could give it a try, but I only have c++ set up at the moment (new laptop) so I have to rely on travis, will take me "some" time. Not sure about js/nbind, maybe @arcanis could do a follow up 😊 please?
emilsjolander commented 2017-01-31 12:24:39 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 java/csharp/objc should be trivial as it is just a simple property setter, just follow what is done for overflow already. I could probably do the javascript stuff unless @arcanis has some time 👍

@woehrl01 java/csharp/objc should be trivial as it is just a simple property setter, just follow what is done for overflow already. I could probably do the javascript stuff unless @arcanis has some time 👍
woehrl01 commented 2017-01-31 12:50:50 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander "some" time 😉 I hope I haven't missed a file. I'll wait on the travis build.

@emilsjolander "some" time 😉 I hope I haven't missed a file. I'll wait on the travis build.
arcanis commented 2017-01-31 12:52:27 -08:00 (Migrated from github.com)
Copy Link

I'm afraid I won't have time to work on this this week, but it shouldn't be much more than copying what is done for setPositionType - same prototype, same concept (cf https://github.com/facebook/yoga/blob/master/javascript/sources/Node.hh#L55, https://github.com/facebook/yoga/blob/master/javascript/sources/Node.cc#L67-L70 and https://github.com/facebook/yoga/blob/master/javascript/sources/nbind.cc#L55). One run of enums.py, another to generate the tests, and that should be it! ... should :)

I'm afraid I won't have time to work on this this week, but it shouldn't be much more than copying what is done for `setPositionType` - same prototype, same concept (cf https://github.com/facebook/yoga/blob/master/javascript/sources/Node.hh#L55, https://github.com/facebook/yoga/blob/master/javascript/sources/Node.cc#L67-L70 and https://github.com/facebook/yoga/blob/master/javascript/sources/nbind.cc#L55). One run of `enums.py`, another to generate the tests, and that should be it! ... should :)
emilsjolander commented 2017-01-31 12:54:42 -08:00 (Migrated from github.com)
Copy Link

@arcanis thanks! That should be enough for me to do this. Don't worry about it

@arcanis thanks! That should be enough for me to do this. Don't worry about it
woehrl01 commented 2017-01-31 12:57:08 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander @arcanis Thanks for the hint. I updated those files, too, let's see if those are enough 👍 😄

@emilsjolander @arcanis Thanks for the hint. I updated those files, too, let's see if those are enough 👍 😄
joce commented 2017-01-31 12:59:33 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 You are a god among men. I can't thank you enough.

@woehrl01 You are a god among men. I can't thank you enough.
woehrl01 commented 2017-01-31 23:36:23 -08:00 (Migrated from github.com)
Copy Link

@joce I'm happy that it fits your needs!

@joce I'm happy that it fits your needs!
emilsjolander (Migrated from github.com) reviewed 2017-02-01 01:58:57 -08:00
emilsjolander (Migrated from github.com) left a comment
Copy Link

Could you add a test to ensure that margin of a hidden node does not effect the next sibling?

Could you add a test to ensure that margin of a hidden node does not effect the next sibling?
gentest/fixtures/YGDisplayTest.html
@@ -0,0 +1,27 @@
<div id="display_none" style="width: 100px; height: 100px; flex-direction: row;">
emilsjolander (Migrated from github.com) commented 2017-02-01 01:53:09 -08:00
Copy Link

All these tests are a bit more complicated than they need to be. For example this first test could be done with just 2 children and have the same effect.

All these tests are a bit more complicated than they need to be. For example this first test could be done with just 2 children and have the same effect.
gentest/fixtures/YGDisplayTest.html
@@ -0,0 +23,4 @@
<div id="display_none_with_position" style="width: 100px; height: 100px; flex-direction: row;">
<div style="flex-grow: 1;"></div>
<div style="flex-grow: 1; display:none; top: 10px;"></div>
emilsjolander (Migrated from github.com) commented 2017-02-01 01:53:46 -08:00
Copy Link

what are you trying to test here?

what are you trying to test here?
java/com/facebook/yoga/YogaNode.java
@@ -310,6 +310,18 @@ public class YogaNode implements YogaNodeAPI<YogaNode> {
jni_YGNodeStyleSetOverflow(mNativePointer, overflow.intValue());
emilsjolander (Migrated from github.com) commented 2017-02-01 01:56:09 -08:00
Copy Link

please use the generated fromInt() method on YogaDisplay. We should be doing the same in other places (i'll push a commit for that now, don't worry about it)

please use the generated fromInt() method on `YogaDisplay`. We should be doing the same in other places (i'll push a commit for that now, don't worry about it)
woehrl01 (Migrated from github.com) reviewed 2017-02-01 04:16:18 -08:00
java/com/facebook/yoga/YogaNode.java
@@ -310,6 +310,18 @@ public class YogaNode implements YogaNodeAPI<YogaNode> {
jni_YGNodeStyleSetOverflow(mNativePointer, overflow.intValue());
woehrl01 (Migrated from github.com) commented 2017-02-01 04:16:17 -08:00
Copy Link

took the changes from GetOverflow and just renamed the name. If you push a commit I'll leave this for now.

took the changes from GetOverflow and just renamed the name. If you push a commit I'll leave this for now.
woehrl01 (Migrated from github.com) reviewed 2017-02-01 04:17:53 -08:00
gentest/fixtures/YGDisplayTest.html
@@ -0,0 +23,4 @@
<div id="display_none_with_position" style="width: 100px; height: 100px; flex-direction: row;">
<div style="flex-grow: 1;"></div>
<div style="flex-grow: 1; display:none; top: 10px;"></div>
woehrl01 (Migrated from github.com) commented 2017-02-01 04:17:53 -08:00
Copy Link

If the position for hidden nodes is 0 or 10. It is 0, so we need to set the postion to 0 in ZeroOutLayoutRecursivly, not sure if this is actually a good idea, that's the reason I didn't implemented it yet. I'm curious what you think about that.

If the position for hidden nodes is 0 or 10. It is 0, so we need to set the postion to 0 in ``` ZeroOutLayoutRecursivly```, not sure if this is actually a good idea, that's the reason I didn't implemented it yet. I'm curious what you think about that.
woehrl01 (Migrated from github.com) reviewed 2017-02-01 04:18:38 -08:00
gentest/fixtures/YGDisplayTest.html
@@ -0,0 +1,27 @@
<div id="display_none" style="width: 100px; height: 100px; flex-direction: row;">
woehrl01 (Migrated from github.com) commented 2017-02-01 04:18:38 -08:00
Copy Link

True, I'll make them simpler/split them up.

True, I'll make them simpler/split them up.
emilsjolander commented 2017-02-02 05:15:49 -08:00 (Migrated from github.com)
Copy Link

Looks good! Thanks for this 👍 👍 👍 👍 👍

Looks good! Thanks for this 👍 👍 👍 👍 👍
🎉 1
facebook-github-bot commented 2017-02-02 05:26:50 -08:00 (Migrated from github.com)
Copy Link

@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/D4501141).

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
CLA Signed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#369
No description provided.
Delete Branch "feature-hidden-nodes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?