Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature hidden nodes #302

Closed
wants to merge 8 commits into from
Closed

Conversation

roxlu
Copy link

@roxlu roxlu commented Dec 23, 2016

No description provided.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@emilsjolander
Copy link
Contributor

I like the suggestion @woehrl01 had in #241 about using the display property from css. So add an enums with values none and flex. What do you think?

@emilsjolander
Copy link
Contributor

@roxlu Are you planning to update this?

@roxlu
Copy link
Author

roxlu commented Jan 5, 2017

@emilsjolander Yes, I'll fix that this weekend. Sorry for not replying earlier.

@emilsjolander
Copy link
Contributor

emilsjolander commented Jan 5, 2017 via email

@joce
Copy link

joce commented Jan 16, 2017

Hey @roxlu This is a feature we'd definitely need. However, having it as a property is is must for us. Tests are also required.
Do you have any plans to update to a property and to write tests? If not, is this something I could help with?

@emilsjolander
Copy link
Contributor

@joce Feel free to put up a PR of your own. @roxlu is probably busy with other things and would love someone else to help out 👍

@joce
Copy link

joce commented Jan 18, 2017

Will look at this then!

@roxlu
Copy link
Author

roxlu commented Jan 19, 2017

Hey @joce and @emilsjolander I've been ill last week and am catching up on work so I won't have any spare time for this until the end of February. @joce my branch with the hide/show feature is here: https://github.com/roxlu/css-layout/tree/feature-hidden-nodes if you want to have a look.

@joce
Copy link

joce commented Jan 24, 2017

A major issue I have however is that I'm on Windows, compiling with VisualStudio, and therefore I do not have a C99 compiler. I would need to have this feature based on a branch that compiles in C++98. Is this workable for you?

@woehrl01
Copy link
Contributor

@joce I had the same problem. compiling on windows works with vs15 community edition.

@joce
Copy link

joce commented Jan 24, 2017

@woehrl01 Excellent! I'll give that a try tonight, then.

@emilsjolander
Copy link
Contributor

@joce any update?

@joce
Copy link

joce commented Jan 31, 2017 via email

@woehrl01
Copy link
Contributor

woehrl01 commented Jan 31, 2017

@joce @emilsjolander I help out with a new PR #369 with the suggested changes. So you can focus on the fires @joce 🔥 🏃 😄

@emilsjolander
Copy link
Contributor

closing this as @woehrl01 has opened up #369

facebook-github-bot pushed a commit that referenced this pull request Feb 6, 2017
Summary:
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.
Closes #369

Reviewed By: astreet

Differential Revision: D4501141

Pulled By: emilsjolander

fbshipit-source-id: 0dfeee381f6d1e4bbba81926126b83dd7abab9d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants