Skip to content

Conversation

@hartbit
Copy link
Contributor

@hartbit hartbit commented Feb 11, 2017

We still need to wait for the YGUnitPoint PR to be merged :) But please let me know what you think. One caveat: because of a limitation of Swift, a literal value can be automatically understood as a point-based YGValue, but variables have to be explicitly cast. I haven't found a way around it yet:

view.yoga.width = 10 // value == 10, unit == YGUnitPixel

let a: CGFloat = 100
view.yoga.height = a // Compiler error
view.yoga.height = YGValue(a) // works, not great

@d16r
Copy link
Contributor

d16r commented Feb 11, 2017

Very cool! I'll be able to merge this in on Monday, can you rebase and update the new CHANGELOG.md file, and list this as breaking change?

#import <yoga/YGEnums.h>
#import <yoga/Yoga.h>

extern YGValue YGPoint(CGFloat value) NS_SWIFT_UNAVAILABLE("Use the swift Int and FloatingPoint extensions instead");
Copy link
Contributor

@d16r d16r Feb 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, lets wrap these:

EXTERN_C_BEGIN

(functions)

EXTERN_C_END

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to do this.

@d16r
Copy link
Contributor

d16r commented Feb 12, 2017

We also need to update this gist so the github docs have been updated.

@d16r
Copy link
Contributor

d16r commented Feb 12, 2017

  • Add EXTERN_C_BEGIN and END to the header.
  • Update the Changelog
  • Add some unit tests. (If you want to put them into a new file, that'd be great. I want to split up YogaKitTests into separate files.
  • (Follow-up) Add a demo sample controller for percentage layout.
  • (Follow-up) Update documentation Gist.

@hartbit
Copy link
Contributor Author

hartbit commented Feb 12, 2017

@dshahidehpour Any ideas for improving the problem with Swift variables?

@d16r
Copy link
Contributor

d16r commented Feb 12, 2017

@hartbit Not yet, but I haven't taken much a look yet.

@d16r
Copy link
Contributor

d16r commented Feb 15, 2017

@hartbit, since Swift doesn't allow implicit casting, I think we might be out of luck...

In the meantime, the Pixel rename has landed (as well as a bunch of changes to YogaKit), so, if you want to rebase, we can move forward this.

I would also really appreciate it if you could add an example to the new Sample App for percentage widths.

@emilsjolander
Copy link
Contributor

@hartbit Do you have an update on this? I would love to get this into the next release.

@hartbit
Copy link
Contributor Author

hartbit commented Feb 27, 2017

@emilsjolander Hello! Sorry, I've been very busy lately. I'll rebase this ASAP.

@emilsjolander
Copy link
Contributor

No worries! Thanks 👍

@hartbit hartbit force-pushed the percentage-values branch from dea31f2 to 9b072e1 Compare March 21, 2017 13:08
@hartbit
Copy link
Contributor Author

hartbit commented Mar 21, 2017

@emilsjolander finally rebased. Sorry for the delay!

@emilsjolander
Copy link
Contributor

I'm on vacation this week. @dshahidehpour could you have a look?

Copy link
Contributor

@d16r d16r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okie, just need to fixup the extern function, and we should be able to merge.

#import <yoga/YGEnums.h>
#import <yoga/Yoga.h>

extern YGValue YGPoint(CGFloat value) NS_SWIFT_UNAVAILABLE("Use the swift Int and FloatingPoint extensions instead");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to do this.

- (void)set##capitalized_name:(CGFloat)lowercased_name \
{ \
YGNodeStyleSet##capitalized_name(self.node, lowercased_name); \
#define YG_VALUE_PROPERTY(lowercased_name, capitalized_name) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this lands, I really want to remove these defines. It might be a bit of extra code, but I'd rather try and use function pointers instead of this.

@facebook-github-bot
Copy link
Contributor

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

@emilsjolander
Copy link
Contributor

@hartbit Sorry this took ages to merge. My bad. Really appreciate this contribution and we are all excited to start using it 👍 👍 👍

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants