WIP: Improve the Objective-C and Swift bridge #289

2016-12-18T13:32:53-08:00

hartbit commented

2016-12-18 13:32:53 -08:00

(Migrated from github.com)

What I've done

Here is what I've done so far:

I created a framework target in the Xcode project to support Carthage
I renamed the prefix from YG to YK to support custom enums
I modified the enums.py generator to add a generation of Objective-C enums
- I removed enums which were not used in the Objective-C bridge
- I made sure not to generate the Count items
- I renamed LTR and RTL to LeftToRight and RightToLeft :-) we don't mind being verbose if it makes identifiers clearer
I transformed the style setters into properties and added the getter methods
I renamed the swift names to drop the prefix, which is not Swift style.

For example, here are what the Objective-C and Swift sample code look like:

Objective-C

- (void)viewDidLoad
{
    UIView *root = self.view;
    root.backgroundColor = [UIColor redColor];
    [root yk_setUsesYoga:YES];
    [root yk_setWidth:self.view.bounds.size.width];
    [root yk_setHeight:self.view.bounds.size.height];
    [root yk_setAlignItems:YKAlignCenter];
    [root yk_setJustifyContent:YKJustifyCenter];

    UIView *child1 = [UIView new];
    child1.backgroundColor = [UIColor blueColor];
    [child1 yk_setUsesYoga:YES];
    [child1 yk_setWidth:100];
    [child1 yk_setHeight:100];

    UIView *child2 = [UIView new];
    child2.backgroundColor = [UIColor greenColor];
    child2.frame = (CGRect) {
        .size = {
            .width = 200,
            .height = 100,
        }
    };

    UIView *child3 = [UIView new];
    child3.backgroundColor = [UIColor yellowColor];
    child3.frame = (CGRect) {
        .size = {
            .width = 100,
            .height = 100,
        }
    };

    [child2 addSubview:child3];
    [root addSubview:child1];
    [root addSubview:child2];
    [root yk_applyLayout];
}

Swift

    override func viewDidLoad() {
        let root = view!
        root.backgroundColor = .red
        root.usesYoga = true
        root.layoutWidth = view.bounds.size.width
        root.layoutHeight = view.bounds.size.height
        root.layoutAlignItems = .center
        root.layoutJustifyContent = .center
      
        let child1 = UIView()
        child1.backgroundColor = .blue
        child1.usesYoga = true
        child1.layoutWidth = 100
        child1.layoutHeight = 100
      
        let child2 = UIView()
        child2.backgroundColor = .green
        child2.frame = CGRect(origin: .zero, size: CGSize(width: 200, height: 100))
      
        let child3 = UIView()
        child3.backgroundColor = .yellow
        child3.frame = CGRect(origin: .zero, size: CGSize(width: 100, height: 100))
      
        child2.addSubview(child3)
        root.addSubview(child1)
        root.addSubview(child2)
        root.applyLayout()
    }

Here's what I'd still like to do, but not sure on how best to achieve:

YKEdge based style values

I have purposefully left the style values which depend on an YKEdge alone because I'd like to improve them to be properties, but I'm undecided on how to. Here are my two solutions:

Normalization

We could create yk_positionLeft, yk_positionRight, etc... properties. The pro is that it's simple and efficient, but it's a bit messy.

Sub-types

Or we could go through sub-types with left, `right, etc.. properties:

@interface YKPosition
@property (nonatomic) CGFloat left;
@property (nonatomic) CGFloat top;
@property (nonatomic) CGFloat right;
@property (nonatomic) CGFloat bottom;
@property (nonatomic) CGFloat start;
@property (nonatomic) CGFloat end;
@property (nonatomic) CGFloat horizontal;
@property (nonatomic) CGFloat vertical;
@property (nonatomic) CGFloat all;
@end

Getting and setting such a property would look like this:

view.yk_position.left = 20;

This would be the cleanest solution. But it forces us to initialise three extra objects per view. We could however restrict this solution to Swift and use a struct, which would reduce the performance cost.

Prefix in Swift

I removed the yk_ prefix in Swift because its an anti-pattern and replaced it with layout. The first question is: would it be better if it were yoga instead?

Whatever it is, I think we still need some kind of prefix to make sure there is no ambiguity that values like width and height are for layout calculations and not values of UIView derived from frame. But I'm not 100% happy of the solution. Like for the edge values, how about a sub-type?

let root = view!
root.backgroundColor = .red
root.yoga.usesYoga = true
root.yoga.width = view.bounds.size.width
root.yoga.height = view.bounds.size.height
root.yoga.alignItems = .center
root.yoga.justifyContent = .center

# What I've done Here is what I've done so far: - I created a framework target in the Xcode project to support Carthage - I renamed the prefix from `YG` to `YK` to support custom enums - I modified the enums.py generator to add a generation of Objective-C enums - I removed enums which were not used in the Objective-C bridge - I made sure not to generate the `Count` items - I renamed LTR and RTL to `LeftToRight` and `RightToLeft` :-) we don't mind being verbose if it makes identifiers clearer - I transformed the style setters into properties and added the getter methods - I renamed the swift names to drop the prefix, which is not Swift style. For example, here are what the Objective-C and Swift sample code look like: ## Objective-C ```objc - (void)viewDidLoad { UIView *root = self.view; root.backgroundColor = [UIColor redColor]; [root yk_setUsesYoga:YES]; [root yk_setWidth:self.view.bounds.size.width]; [root yk_setHeight:self.view.bounds.size.height]; [root yk_setAlignItems:YKAlignCenter]; [root yk_setJustifyContent:YKJustifyCenter]; UIView *child1 = [UIView new]; child1.backgroundColor = [UIColor blueColor]; [child1 yk_setUsesYoga:YES]; [child1 yk_setWidth:100]; [child1 yk_setHeight:100]; UIView *child2 = [UIView new]; child2.backgroundColor = [UIColor greenColor]; child2.frame = (CGRect) { .size = { .width = 200, .height = 100, } }; UIView *child3 = [UIView new]; child3.backgroundColor = [UIColor yellowColor]; child3.frame = (CGRect) { .size = { .width = 100, .height = 100, } }; [child2 addSubview:child3]; [root addSubview:child1]; [root addSubview:child2]; [root yk_applyLayout]; } ``` ## Swift ```swift override func viewDidLoad() { let root = view! root.backgroundColor = .red root.usesYoga = true root.layoutWidth = view.bounds.size.width root.layoutHeight = view.bounds.size.height root.layoutAlignItems = .center root.layoutJustifyContent = .center let child1 = UIView() child1.backgroundColor = .blue child1.usesYoga = true child1.layoutWidth = 100 child1.layoutHeight = 100 let child2 = UIView() child2.backgroundColor = .green child2.frame = CGRect(origin: .zero, size: CGSize(width: 200, height: 100)) let child3 = UIView() child3.backgroundColor = .yellow child3.frame = CGRect(origin: .zero, size: CGSize(width: 100, height: 100)) child2.addSubview(child3) root.addSubview(child1) root.addSubview(child2) root.applyLayout() } ``` Here's what I'd still like to do, but not sure on how best to achieve: # YKEdge based style values I have purposefully left the style values which depend on an `YKEdge` alone because I'd like to improve them to be properties, but I'm undecided on how to. Here are my two solutions: ## Normalization We could create `yk_positionLeft`, `yk_positionRight`, etc... properties. The pro is that it's simple and efficient, but it's a bit messy. ## Sub-types Or we could go through sub-types with `left`, `right, etc.. properties: ``` @interface YKPosition @property (nonatomic) CGFloat left; @property (nonatomic) CGFloat top; @property (nonatomic) CGFloat right; @property (nonatomic) CGFloat bottom; @property (nonatomic) CGFloat start; @property (nonatomic) CGFloat end; @property (nonatomic) CGFloat horizontal; @property (nonatomic) CGFloat vertical; @property (nonatomic) CGFloat all; @end ``` Getting and setting such a property would look like this: ``` view.yk_position.left = 20; ``` This would be the cleanest solution. But it forces us to initialise three extra objects per view. We could however restrict this solution to Swift and use a struct, which would reduce the performance cost. # Prefix in Swift I removed the `yk_` prefix in Swift because its an anti-pattern and replaced it with `layout`. The first question is: would it be better if it were `yoga` instead? Whatever it is, I think we still need some kind of prefix to make sure there is no ambiguity that values like `width` and `height` are for layout calculations and not values of `UIView` derived from `frame`. But I'm not 100% happy of the solution. Like for the edge values, how about a sub-type? ``` let root = view! root.backgroundColor = .red root.yoga.usesYoga = true root.yoga.width = view.bounds.size.width root.yoga.height = view.bounds.size.height root.yoga.alignItems = .center root.yoga.justifyContent = .center ```

facebook-github-bot commented

2016-12-18 13:35:04 -08:00

(Migrated from github.com)

@hartbit updated the pull request - view changes

@hartbit updated the pull request - [view changes](https://github.com/facebook/yoga/pull/289/files/ec31ea0bc99da5eefa08a807961e249157bf1ed8..f24baffffab98cc0d4e813be9d7d0c6271f1bb29)

ghost commented

2016-12-18 13:59:28 -08:00

(Migrated from github.com)

Haven't tried it yet, but it looks cool!

As far as the swift prefix goes I am in favor of yogaWidth style simply because Xcode autocompletion will immediately pick it up upon typing width for example. Otherwise you would have to type .yoga. before the autocomplete kicks in.

I am in favor of YKEdge property names like yogaPositionLeft mainly for the same reason.

Should your SwiftViewController class be named YogaViewController to indicate its function/purpose?

Haven't tried it yet, but it looks cool! As far as the swift prefix goes I am in favor of `yogaWidth` style simply because Xcode autocompletion will immediately pick it up upon typing `width` for example. Otherwise you would have to type `.yoga.` before the autocomplete kicks in. I am in favor of YKEdge property names like `yogaPositionLeft` mainly for the same reason. Should your SwiftViewController class be named `YogaViewController` to indicate its function/purpose?

hartbit commented

2016-12-18 14:07:50 -08:00

(Migrated from github.com)

I am in favor of YKEdge property names like yogaPositionLeft mainly for the same reason.

Part of me wants to agree with you. But another part of me feels like this would feel foreign. For example, if we follow your argument to the end, we wouldn't have view.bounds.size.width but view.boundsSizeWidth.

> I am in favor of YKEdge property names like yogaPositionLeft mainly for the same reason. Part of me wants to agree with you. But another part of me feels like this would feel foreign. For example, if we follow your argument to the end, we wouldn't have `view.bounds.size.width` but `view.boundsSizeWidth`.

ghost commented

2016-12-18 14:09:10 -08:00

(Migrated from github.com)

Good point. Maybe they just need to make autocomplete better :-)

hartbit commented

2016-12-18 14:11:16 -08:00

(Migrated from github.com)

Should your SwiftViewController class be named YogaViewController to indicate its function/purpose?

Concerning this, I don't mind changing it. But then I don't see why the Objective-C class should keep it's ViewController name. They are both yoga :)

> Should your SwiftViewController class be named YogaViewController to indicate its function/purpose? Concerning this, I don't mind changing it. But then I don't see why the Objective-C class should keep it's `ViewController` name. They are both yoga :)

ghost commented

2016-12-18 14:15:13 -08:00

(Migrated from github.com)

Correct again. I had not see it in context with the objc class. The name just seemed generic when I saw it in the commit. It's fine as is.

hartbit commented

2016-12-18 14:32:25 -08:00

(Migrated from github.com)

The more I think about it, the more I converge to this solution:

Make the YGNodeBridge class public and rename it to YKLayout
Make its accessor public and rename it to layout
Move all the extension properties and methods to it
Remove the yk_ prefixes on properties and methods
Create properties for all position, margin, and padding values

This allows us to "namespace" all properties and methods so they are not confused with UIView properties (width and height). Because we've removed the ambiguity, we can safely remove the yk_ prefix.

The result would look like this:

Objective-C

- (void)viewDidLoad
{
    UIView *root = self.view;
    root.backgroundColor = [UIColor redColor];
    root.layout.enabled = YES;
    root.layout.width = self.view.bounds.size.width;
    root.layout.height = self.view.bounds.size.height;
    root.layout.alignItems = YKAlignCenter;
    root.layout.justifyContent = YKJustifyCenter;
    root.layout.paddingLeft = 5;
    root.layout.marginAll = 10;
    [root.layout apply];
}

Swift

override func viewDidLoad() {
    let root = view!
    root.backgroundColor = .red
    root.layout.enabled = true
    root.layout.width = view.bounds.size.width
    root.layout.height = view.bounds.size.height
    root.layout.alignItems = .center
    root.layout.justifyContent = .center
    root.layout.paddingLeft = 5
    root.layout.marginAll = 10
    root.layout.apply()
}

Post Scriptum

In my example, I renamed usesYoga to enabled. It makes more sense once on the YKLayout class to go for a more straight-forward property name.
I think it would be best to keep layout as a property name. I can see this library be used as a dependency in other libraries to React-ify iOS and mentioning yoga could confuse users who are not directly depending on yoga.

The more I think about it, the more I converge to this solution: - Make the `YGNodeBridge` class public and rename it to `YKLayout` - Make its accessor public and rename it to `layout` - Move all the extension properties and methods to it - Remove the `yk_` prefixes on properties and methods - Create properties for all `position`, `margin`, and `padding` values This allows us to "namespace" all properties and methods so they are not confused with `UIView` properties (width and height). Because we've removed the ambiguity, we can safely remove the `yk_` prefix. The result would look like this: ## Objective-C ```objc - (void)viewDidLoad { UIView *root = self.view; root.backgroundColor = [UIColor redColor]; root.layout.enabled = YES; root.layout.width = self.view.bounds.size.width; root.layout.height = self.view.bounds.size.height; root.layout.alignItems = YKAlignCenter; root.layout.justifyContent = YKJustifyCenter; root.layout.paddingLeft = 5; root.layout.marginAll = 10; [root.layout apply]; } ``` ## Swift ```swift override func viewDidLoad() { let root = view! root.backgroundColor = .red root.layout.enabled = true root.layout.width = view.bounds.size.width root.layout.height = view.bounds.size.height root.layout.alignItems = .center root.layout.justifyContent = .center root.layout.paddingLeft = 5 root.layout.marginAll = 10 root.layout.apply() } ``` # Post Scriptum 1. In my example, I renamed `usesYoga` to `enabled`. It makes more sense once on the `YKLayout` class to go for a more straight-forward property name. 2. I think it would be best to keep `layout` as a property name. I can see this library be used as a dependency in other libraries to React-ify iOS and mentioning `yoga` could confuse users who are not directly depending on `yoga`.

ghost commented

2016-12-18 14:40:46 -08:00

(Migrated from github.com)

So the edge values would look like root.layout.margin.bottom = 20 then.

Looks clean. I like it. I like layout better thanyoga as well.

So the edge values would look like `root.layout.margin.bottom = 20` then. Looks clean. I like it. I like `layout` better than`yoga` as well.

hartbit commented

2016-12-18 14:42:36 -08:00

(Migrated from github.com)

So the edge values would look like root.layout.margin.bottom = 20 then.

No, as you can see in the example, I decided to go for root.layout.marginBottom instead. But I'm not completely decided yet. Let's wait to see what @emilsjolander has to say :)

> So the edge values would look like root.layout.margin.bottom = 20 then. No, as you can see in the example, I decided to go for `root.layout.marginBottom` instead. But I'm not completely decided yet. Let's wait to see what @emilsjolander has to say :)

ghost commented

2016-12-18 14:46:18 -08:00

(Migrated from github.com)

I didn't see an example of a coded edge value in your example so I was just trying to clarify.

hartbit commented

2016-12-18 14:48:05 -08:00

(Migrated from github.com)

Can you see the root.layout.paddingLeft = 5 and root.layout.marginAll = 10 lines in my example? I updated some parts of the code after submitting the comment. Perhaps you are seeing an old version.

Can you see the `root.layout.paddingLeft = 5` and `root.layout.marginAll = 10` lines in my example? I updated some parts of the code after submitting the comment. Perhaps you are seeing an old version.

ghost commented

2016-12-18 14:51:11 -08:00

(Migrated from github.com)

That explains it because there was definitely not an example of an edge value there.

hartbit commented

2016-12-18 15:08:06 -08:00

(Migrated from github.com)

Side note: YGUndefined (NAN) would be better expressed as optional values in Swift. But changing that would require us to redefine all value properties in Swift like this:

var flexGrow: CGFloat? {
    get {
        let flexGrow = YGNodeStyleGetFlexGrow(self)
        return flexGrow != YGUndefined ? flexGrow : nil
    }
    set {
        YGNodeStyleSetFlexGrow(self, flexGrow ?? YGUndefined)
    }
}

Side note: `YGUndefined` (NAN) would be better expressed as optional values in Swift. But changing that would require us to redefine all value properties in Swift like this: ``` var flexGrow: CGFloat? { get { let flexGrow = YGNodeStyleGetFlexGrow(self) return flexGrow != YGUndefined ? flexGrow : nil } set { YGNodeStyleSetFlexGrow(self, flexGrow ?? YGUndefined) } } ```

facebook-github-bot commented

2016-12-18 16:14:04 -08:00

(Migrated from github.com)

@hartbit updated the pull request - view changes

@hartbit updated the pull request - [view changes](https://github.com/facebook/yoga/pull/289/files/f24baffffab98cc0d4e813be9d7d0c6271f1bb29..5f5e5df32808c773fe1c2615388d49f6eb4c40f0)

hartbit commented

2016-12-18 16:14:06 -08:00

(Migrated from github.com)

Ok, I implemented what I presented previous, except for the optional values in Swift.

emilsjolander commented

2016-12-18 19:47:56 -08:00

(Migrated from github.com)

This is cool! Thanks 👍 Couple issues with the API before I start reviewing the code.

I never understood why the prefix needed to be changed? But as you no longer use it that does not really matter.
I think using optionals in swift would be good, not a must to merge this though but a cool future improvement.
marginBottom vs margin.bottom. I prefer margin.bottom as I imagine this will lead to less code but have no strong preference.
Adding a property named layout to UIView can be confusing for 2 reasons.
- It is not prefixed and thus can be very confusing if it is imported as a transient dependency. I much prefer calling the property yoga. If another library wants to hide this behind the name layout that is possible for them to do if they choose to.
- Layout is a concept in Yoga already. Yoga differentiates between style and layout properties. layout properties are the outputs of the calculation (what is stores in the UIView frame). There is already out such layout output that cannot be stored in the frame (computed layout direction) and there could potentially be more in the future. Also keeping naming consistent with the C implementation here helps understanding.

This is cool! Thanks 👍 Couple issues with the API before I start reviewing the code. 1. I never understood why the prefix needed to be changed? But as you no longer use it that does not really matter. 2. I think using optionals in swift would be good, not a must to merge this though but a cool future improvement. 3. `marginBottom` vs `margin.bottom`. I prefer `margin.bottom` as I imagine this will lead to less code but have no strong preference. 4. Adding a property named `layout` to `UIView` can be confusing for 2 reasons. - It is not prefixed and thus can be very confusing if it is imported as a transient dependency. I much prefer calling the property `yoga`. If another library wants to hide this behind the name `layout` that is possible for them to do if they choose to. - Layout is a concept in Yoga already. Yoga differentiates between `style` and `layout` properties. `layout` properties are the outputs of the calculation (what is stores in the `UIView` frame). There is already out such layout output that cannot be stored in the frame (computed layout direction) and there could potentially be more in the future. Also keeping naming consistent with the C implementation here helps understanding.

hartbit commented

2016-12-18 22:44:15 -08:00

(Migrated from github.com)

It I kept the original prefix, there would have been a symbol collision between the C enums and the Objectice-C enum in the implementation file. If you know of another way around that, we can go back to the original YG.
That's what I thought also. One thing at a time :)
Yep, let's do that. I'll try implementing that.
A few comments:
- Why would it be confusing if imported through as a transient dependency? I would argue the reverse is true. If I depend on a hypothetical ReactSwift framework, the layout of my components makes sense, but the yoga of my components feels weird. Moreover, the property can't be hidden from a Swift transitent dependency: there is no way I know to import a module as a private dependency. Perhaps you can correct me on that.
- That argument makes sense from the standpoint of a yoga maintainer. But from the point of view of a user of the Swift or Objective-C bridge, that distinction doesn't seem to exist as we only mainly manipulate styles.
- I just wanted to play devil's advocate in the above two points. If you still feel strongly about changing the property to yoga, I will oblige :)

1. It I kept the original prefix, there would have been a symbol collision between the C enums and the Objectice-C enum in the implementation file. If you know of another way around that, we can go back to the original `YG`. 2. That's what I thought also. One thing at a time :) 3. Yep, let's do that. I'll try implementing that. 4. A few comments: * Why would it be confusing if imported through as a transient dependency? I would argue the reverse is true. If I depend on a hypothetical ReactSwift framework, the `layout` of my components makes sense, but the `yoga` of my components feels weird. Moreover, the property can't be hidden from a Swift transitent dependency: there is no way I know to import a module as a private dependency. Perhaps you can correct me on that. * That argument makes sense from the standpoint of a yoga maintainer. But from the point of view of a user of the Swift or Objective-C bridge, that distinction doesn't seem to exist as we only mainly manipulate styles. * I just wanted to play devil's advocate in the above two points. If you still feel strongly about changing the property to `yoga`, I will oblige :)

emilsjolander commented

2016-12-18 22:49:28 -08:00

(Migrated from github.com)

It I kept the original prefix, there would have been a symbol collision between the C enums and the Objectice-C enum in the implementation file. If you know of another way around that, we can go back to the original YG.

Why can't they use the same definitions? I feel strongly about keeping the same prefix. Let's find a solution for this.

Why would it be confusing if imported through as a transient dependency? I would argue the reverse is true. If I depend on a hypothetical ReactSwift framework, the layout of my components makes sense, but the yoga of my components feels weird. Moreover, the property can't be hidden from a Swift transitent dependency: there is no way I know to import a module as a private dependency. Perhaps you can correct me on that.

It cannot be hidden, wrong wording, sorry :) What i meant was that ReactSwift could expose the property as layout via an alias if it wanted to. Or it could just make it clear that it uses yoga for layout and then it would not be weird. layout on UIView also has a way to high chance of being used by apple in the future so let's avoid it.

That argument makes sense from the standpoint of a yoga maintainer. But from the point of view of a user of the Swift or Objective-C bridge, that distinction doesn't seem to exist as we only mainly manipulate styles.

Fair point. But we may add more layout outputs in the future and then it could potentially become confusing.

> It I kept the original prefix, there would have been a symbol collision between the C enums and the Objectice-C enum in the implementation file. If you know of another way around that, we can go back to the original YG. Why can't they use the same definitions? I feel strongly about keeping the same prefix. Let's find a solution for this. > Why would it be confusing if imported through as a transient dependency? I would argue the reverse is true. If I depend on a hypothetical ReactSwift framework, the layout of my components makes sense, but the yoga of my components feels weird. Moreover, the property can't be hidden from a Swift transitent dependency: there is no way I know to import a module as a private dependency. Perhaps you can correct me on that. It cannot be hidden, wrong wording, sorry :) What i meant was that ReactSwift could expose the property as `layout` via an alias if it wanted to. Or it could just make it clear that it uses yoga for layout and then it would not be weird. `layout` on UIView also has a way to high chance of being used by apple in the future so let's avoid it. > That argument makes sense from the standpoint of a yoga maintainer. But from the point of view of a user of the Swift or Objective-C bridge, that distinction doesn't seem to exist as we only mainly manipulate styles. Fair point. But we may add more layout outputs in the future and then it could potentially become confusing.

hartbit commented

2016-12-18 23:30:31 -08:00

(Migrated from github.com)

Why can't they use the same definitions? I feel strongly about keeping the same prefix. Let's find a solution for this.

I didn't want to use the same definitions to remove the Count item. It really feels odd to me to leak an implementation detail into the bridges. Ok, here's a potential solution: why don't be modify the enums.py to remove Count from the C enums and redefine it as a constant in a private header?

// In YGEnums.h
typedef enum YGFlexDirection {
  YGFlexDirectionColumn,
  YGFlexDirectionColumnReverse,
  YGFlexDirectionRow,
  YGFlexDirectionRowReverse,
} YGFlexDirection;

// In YGEnums-Private.h
const int YGFlexDirectionCount = 4;

It cannot be hidden, wrong wording, sorry :) What i meant was that ReactSwift could expose the property as layout via an alias if it wanted to. Or it could just make it clear that it uses yoga for layout and then it would not be weird. layout on UIView also has a way to high chance of being used by apple in the future so let's avoid it.

Okay, let's go for yoga :)

> Why can't they use the same definitions? I feel strongly about keeping the same prefix. Let's find a solution for this. I didn't want to use the same definitions to remove the `Count` item. It really feels odd to me to leak an implementation detail into the bridges. Ok, here's a potential solution: why don't be modify the `enums.py` to remove `Count` from the C enums and redefine it as a constant in a private header? ``` // In YGEnums.h typedef enum YGFlexDirection { YGFlexDirectionColumn, YGFlexDirectionColumnReverse, YGFlexDirectionRow, YGFlexDirectionRowReverse, } YGFlexDirection; // In YGEnums-Private.h const int YGFlexDirectionCount = 4; ``` > It cannot be hidden, wrong wording, sorry :) What i meant was that ReactSwift could expose the property as layout via an alias if it wanted to. Or it could just make it clear that it uses yoga for layout and then it would not be weird. layout on UIView also has a way to high chance of being used by apple in the future so let's avoid it. Okay, let's go for `yoga` :)

emilsjolander commented

2016-12-18 23:33:00 -08:00

(Migrated from github.com)

Ok, here's a potential solution: why don't be modify the enums.py to remove Count from the C enums and redefine it as a constant in a private header?

This sounds great! Could you do that as a separate PR?

> Ok, here's a potential solution: why don't be modify the enums.py to remove Count from the C enums and redefine it as a constant in a private header? This sounds great! Could you do that as a separate PR?

facebook-github-bot commented

2016-12-19 10:53:26 -08:00

(Migrated from github.com)

@hartbit updated the pull request - view changes

@hartbit updated the pull request - [view changes](https://github.com/facebook/yoga/pull/289/files/5f5e5df32808c773fe1c2615388d49f6eb4c40f0..7158fc24a3c50ef89dd849ec591c22a174035f42)

hartbit commented

2016-12-19 11:10:48 -08:00

(Migrated from github.com)

@emilsjolander Done: #292

emilsjolander commented

2016-12-20 00:43:20 -08:00

(Migrated from github.com)

@hartbit This look like we can merge it very soon 👍 Thanks for all the work

Revert back to YG prefix
layout -> yoga
apply -> applyLayout (more explicit now that the prop is no longer called layout)
Mark YogaKit/UIView+YogaKit.{h,m} as a move from YogaKit/UIView+Yoga.{h,m}

@hartbit This look like we can merge it very soon 👍 Thanks for all the work - Revert back to YG prefix - `layout` -> `yoga` - `apply` -> `applyLayout` (more explicit now that the prop is no longer called `layout`) - Mark `YogaKit/UIView+YogaKit.{h,m}` as a move from `YogaKit/UIView+Yoga.{h,m}`

hartbit commented

2016-12-20 00:56:16 -08:00

(Migrated from github.com)

@emilsjolander There's still the question of edge values. Currently, I have them as marginBottom, not margin.bottom. If what you are worried of is duplication of code, that's not a problem, because I used macros (like in the C implementation). I think it's more a question of style.

@emilsjolander There's still the question of edge values. Currently, I have them as `marginBottom`, not `margin.bottom`. If what you are worried of is duplication of code, that's not a problem, because I used macros (like in the C implementation). I think it's more a question of style.

emilsjolander commented

2016-12-20 00:59:49 -08:00

(Migrated from github.com)

Ah yes I forgot about that one. I prefer margin.bottom because that would allow margin/border/padding to share a data structure which would allow for writing more generic code which could take this structure as a parameter.

Ah yes I forgot about that one. I prefer `margin.bottom` because that would allow margin/border/padding to share a data structure which would allow for writing more generic code which could take this structure as a parameter.

👍 1

hartbit commented

2016-12-20 01:04:47 -08:00

(Migrated from github.com)

@emilsjolander Ok, I just had a discussion with a colleague at work that has done a fair amount of ReactNative and he'd propose following their syntax:

https://facebook.github.io/react-native/docs/layout-props.html

That would mean:

margin instead of marginAll
marginLeft, etc...

Is that argument worth thinking about, or do we really want a separate data structure?

@emilsjolander Ok, I just had a discussion with a colleague at work that has done a fair amount of ReactNative and he'd propose following their syntax: https://facebook.github.io/react-native/docs/layout-props.html That would mean: - `margin` instead of `marginAll` - `marginLeft`, etc... Is that argument worth thinking about, or do we really want a separate data structure?

emilsjolander commented

2016-12-20 01:11:47 -08:00

(Migrated from github.com)

@hartbit I'm Ok with that, I don't feel strongly about this.

hartbit commented

2016-12-20 01:14:37 -08:00

(Migrated from github.com)

@emilsjolander I'll have the modifications done this evening. Thanks for the feedback!

emilsjolander commented

2016-12-20 01:15:18 -08:00

(Migrated from github.com)

@hartbit Awesome. I'm trying to merge #292 as we speak

d16r commented

2016-12-20 05:32:52 -08:00

(Migrated from github.com)

Hey @hartbit! Thank you so much for all your work on this!

I would love to start merging some of these changes in, but I was wondering, would it be possible to start breaking them up into smaller pull requests? I think some of these features we can ship right away (e170dde, f24baff), and it will the review of the other commits much easier.

Unfortunately, the tool we use review/merge changes would bundle all these commits together into a massive one, and I rather break it up it into pieces :)

Hey @hartbit! Thank you so much for all your work on this! I would love to start merging some of these changes in, but I was wondering, would it be possible to start breaking them up into smaller pull requests? I think some of these features we can ship right away (e170dde, f24baff), and it will the review of the other commits much easier. Unfortunately, the tool we use review/merge changes would bundle all these commits together into a massive one, and I rather break it up it into pieces :)

d16r (Migrated from github.com) reviewed 2016-12-20 09:32:23 -08:00

d16r (Migrated from github.com) left a comment

The code is looking really good! It gave me a thought though...

This feels like this is turning into LayoutKit. If the YKLayout is going to hang on to view internally to apply the layout to it...it feels like we are a step away from:

Removing the UIView category.
Adding an API to YKLayout so that we can subclass it and have it pumped out configured views.
Pass around YKLayouts to create view hierarchies.

And this is almost exactly what LayoutKit does...
Just playing devil's advocate here, but, in the meantime I think we should push forward!

The code is looking really good! It gave me a thought though... This feels like this is turning into [LayoutKit](http://layoutkit.org/#hello-world). If the `YKLayout` is going to hang on to view internally to apply the layout to it...it feels like we are a step away from: 1. Removing the `UIView` category. 2. Adding an API to `YKLayout` so that we can subclass it and have it pumped out configured views. 3. Pass around `YKLayout`s to create view hierarchies. And this is almost exactly what `LayoutKit` does... Just playing devil's advocate here, but, in the meantime I think we should push forward!

YogaKit/YKLayout.h

						
				@@ -0,0 +40,4 @@

				@property (nonatomic) CGFloat positionBottom;

				@property (nonatomic) CGFloat positionStart;

				@property (nonatomic) CGFloat positionEnd;

				@property (nonatomic) CGFloat positionHorizontal;

d16r (Migrated from github.com) commented

2016-12-20 09:00:23 -08:00

Personally, I find this makes the API much more confusing. There are multiple properties that essentially do the same thing. I could set positionLeft and positionRight or just simply set positionHorizontal. To me, it would be much more idiomatic to Objective-C and Swift if we did something like this.

@property (nonatomic, readwrite, assign) UIEdgeInsets position;

or

- (void)setMargin:(CGFloat)margin forEdges:(YKEdges)edges;

where YKEdges is an NSOption.

I would apply this comment to margin and padding too. cc: @emilsjolander

Personally, I find this makes the API much more confusing. There are multiple properties that essentially do the same thing. I could set `positionLeft` and `positionRight` or just simply set `positionHorizontal`. To me, it would be much more idiomatic to Objective-C and Swift if we did something like this. ```objc @property (nonatomic, readwrite, assign) UIEdgeInsets position; ``` or ```objc - (void)setMargin:(CGFloat)margin forEdges:(YKEdges)edges; ``` where YKEdges is an NSOption. I would apply this comment to `margin` and `padding` too. cc: @emilsjolander

YogaKit/YKLayout.h

						
				@@ -0,0 +58,4 @@

				@property (nonatomic) CGFloat paddingTop;

				@property (nonatomic) CGFloat paddingRight;

				@property (nonatomic) CGFloat paddingBottom;

				@property (nonatomic) CGFloat paddingStart;

d16r (Migrated from github.com) commented

2016-12-20 09:02:54 -08:00

nit across the board, can we explicity add all the properties? i.e.

@property (nonatomic, readwrite, assign) ...

nit across the board, can we explicity add all the properties? i.e. ``` @property (nonatomic, readwrite, assign) ... ```

hartbit (Migrated from github.com) reviewed 2016-12-20 13:00:44 -08:00

YogaKit/YKLayout.h

						
				@@ -0,0 +40,4 @@

				@property (nonatomic) CGFloat positionBottom;

				@property (nonatomic) CGFloat positionStart;

				@property (nonatomic) CGFloat positionEnd;

				@property (nonatomic) CGFloat positionHorizontal;

hartbit (Migrated from github.com) commented

2016-12-20 13:00:44 -08:00

@dshahidehpour I agree the solution I have is not ideal, but I fail to see how UIEdgeInsets or a NS_OPTIONS YKEdges would allow us to express the Start and End values.

@dshahidehpour I agree the solution I have is not ideal, but I fail to see how `UIEdgeInsets` or a `NS_OPTIONS YKEdges` would allow us to express the `Start` and `End` values.

hartbit (Migrated from github.com) reviewed 2016-12-20 13:00:55 -08:00

YogaKit/YKLayout.h

						
				@@ -0,0 +58,4 @@

				@property (nonatomic) CGFloat paddingTop;

				@property (nonatomic) CGFloat paddingRight;

				@property (nonatomic) CGFloat paddingBottom;

				@property (nonatomic) CGFloat paddingStart;

hartbit (Migrated from github.com) commented

2016-12-20 13:00:55 -08:00

Sure!

hartbit commented

2016-12-20 14:27:57 -08:00

(Migrated from github.com)

@dshahidehpour I've opened a pull request for the gitignore fix (#294). I've having a few issues concerning the Carthage PR but I'll open something soon.

👍 1

d16r (Migrated from github.com) reviewed 2016-12-20 15:29:12 -08:00

YogaKit/YKLayout.h

						
				@@ -0,0 +40,4 @@

				@property (nonatomic) CGFloat positionBottom;

				@property (nonatomic) CGFloat positionStart;

				@property (nonatomic) CGFloat positionEnd;

				@property (nonatomic) CGFloat positionHorizontal;

d16r (Migrated from github.com) commented

2016-12-20 15:29:12 -08:00

@hartbit Whoops, I didn't even notice those, do we think they need to part of YogaKit? I purposely left some properties out at first because I didn't see a huge use-case for them with UIKit.

@hartbit Whoops, I didn't even notice those, do we think they need to part of `YogaKit`? I purposely left some properties out at first because I didn't see a huge use-case for them with UIKit.

hartbit (Migrated from github.com) reviewed 2016-12-20 16:21:51 -08:00

YogaKit/YKLayout.h

						
				@@ -0,0 +40,4 @@

				@property (nonatomic) CGFloat positionBottom;

				@property (nonatomic) CGFloat positionStart;

				@property (nonatomic) CGFloat positionEnd;

				@property (nonatomic) CGFloat positionHorizontal;

hartbit (Migrated from github.com) commented

2016-12-20 16:21:51 -08:00

@dshahidehpour The Yoga website says: "We believe that Right-to-Left (RTL) should be a first class citizen when it comes to layout" and I strongly agree. UIKit also supports it by default with leading and trailing constraints in Auto Layout and I'd be very sorry to have them out of YogaKit.

You mention that there are multiple properties that do the same thing. But it's not unheard of in Cocoa. For example, a UIView's frame.size is the same as its bounds.size. Setting one affects the other. And it's the same with it's center, which is the mid-x and mid-y of frame.

I know, this is a difficult design decision. But I think there is no perfect solution. For sake of discussion, here are the solutions I see, as well as their pros and cons.

Properties

@property (nonatomic) CGFloat paddingLeft;
@property (nonatomic) CGFloat paddingRight;
@property (nonatomic) CGFloat paddingTop;
@property (nonatomic) CGFloat paddingBottom;
@property (nonatomic) CGFloat paddingStart;
@property (nonatomic) CGFloat paddingEnd;
@property (nonatomic) CGFloat paddingHorizontal;
@property (nonatomic) CGFloat paddingVertical;
@property (nonatomic) CGFloat padding; // I've removed the All because it's a change I'd like to do

Usage in Objective-C

view.layout.padding = 15;
// OR
view.layout.paddingTop = 10;
// OR
view.layout.paddingStart = 10;
// OR
view.layout.paddingHorizontal = 10;
view.layout.paddingTop = 15;

Usage in Swift

view.layout.padding = 15
// OR
view.layout.paddingTop = 10
// OR
view.layout.paddingStart = 10
// OR
view.layout.paddingHorizontal = 10
view.layout.paddingTop = 10

Pros

Recognizable API for people from CSS and React Native (it's actually the exact same API as React Native).
Auto-completion makes this API version fast to use, as for all other properties.

Cons

Makes the API surface larger, not very DRY.

Methods with `NS_ENUM`

- (CGFloat)paddingForEdge:(YGEdge)edge;
- (void)setPadding:(CGFloat)padding forEdge:(YGEdge)edge;

Usage in Objective-C

[view.layout setPadding:15 forEdge:YGEdgeAll];
// OR
[view.layout setPadding:10 forEdge:YGEdgeTop];
// OR
// NO WAY TO SET START
[view.layout setPadding:10 forEdge:YKEdgeHorizontal];
[view.layout setPadding:10 forEdge:YKEdgeTop];

Usage in Swift

view.layout.setPadding(15, for: .all)
// OR
view.layout.setPadding(10, for: .top)
// OR
// NO WAY TO SET START
// OR
view.layout.setPadding(10, for: .horizontal)
view.layout.setPadding(10, for: .top)

Pros

The usage looks cleaner by having a smaller API surface area.

Cons

We can't use dot-notation like all other properties, which breaks the reading flow. Perhaps it's my personal taste, but I see that as a big minus.

root.layout.width = 5;
[root.layout setPadding:10 forEdge:YGEdgeTop];
root.layout.flexGrow = 1;

`YGEdgeValue`


@interface YGEdgeValue
@property (nonatomic) CGFloat left;
@property (nonatomic) CGFloat right;
@property (nonatomic) CGFloat top;
@property (nonatomic) CGFloat bottom;
@property (nonatomic) CGFloat start;
@property (nonatomic) CGFloat end;
@property (nonatomic) CGFloat horizontal;
@property (nonatomic) CGFloat vertical;
@property (nonatomic) CGFloat all;
@end

@inteface YGLayout
@property (nonatomic, readonly) YGEdgeValue *padding;
@end

Usage in Objective-C

view.layout.padding.all = 15;
// OR
view.layout.padding.top = 10;
// OR
view.layout.padding.start = 10;
// OR
view.layout.padding.horizontal = 10;
view.layout.padding.top = 15;

Usage in Swift

view.layout.padding.all = 15
// OR
view.layout.padding.top = 10
// OR
view.layout.padding.start = 10
// OR
view.layout.padding.horizontal = 10
view.layout.padding.top = 10

Pros and Cons

This is basically a different flavor of properties. I'm not a hug fan of it because it multiplies the number of objects on the heap that yoga generates by 4 for a small gain in readability and API surface and a small loss in usage typing speed because having to go through two auto-completion hints (padding, then top).

Conclusion

On and all, here's my ranking of preference:

Properties
YGEdgeValue
Methods

@dshahidehpour The Yoga website says: "We believe that Right-to-Left (RTL) should be a first class citizen when it comes to layout" and I strongly agree. UIKit also supports it by default with leading and trailing constraints in Auto Layout and I'd be very sorry to have them out of YogaKit. You mention that there are multiple properties that do the same thing. But it's not unheard of in Cocoa. For example, a UIView's `frame.size` is the same as its `bounds.size`. Setting one affects the other. And it's the same with it's `center`, which is the mid-x and mid-y of `frame`. I know, this is a difficult design decision. But I think there is no perfect solution. For sake of discussion, here are the solutions I see, as well as their pros and cons. # Properties ``` @property (nonatomic) CGFloat paddingLeft; @property (nonatomic) CGFloat paddingRight; @property (nonatomic) CGFloat paddingTop; @property (nonatomic) CGFloat paddingBottom; @property (nonatomic) CGFloat paddingStart; @property (nonatomic) CGFloat paddingEnd; @property (nonatomic) CGFloat paddingHorizontal; @property (nonatomic) CGFloat paddingVertical; @property (nonatomic) CGFloat padding; // I've removed the All because it's a change I'd like to do ``` ## Usage in Objective-C ``` view.layout.padding = 15; // OR view.layout.paddingTop = 10; // OR view.layout.paddingStart = 10; // OR view.layout.paddingHorizontal = 10; view.layout.paddingTop = 15; ``` ## Usage in Swift ``` view.layout.padding = 15 // OR view.layout.paddingTop = 10 // OR view.layout.paddingStart = 10 // OR view.layout.paddingHorizontal = 10 view.layout.paddingTop = 10 ``` ## Pros * Recognizable API for people from CSS and React Native (it's actually the exact same API as React Native). * Auto-completion makes this API version fast to use, as for all other properties. ## Cons * Makes the API surface larger, not very DRY. # Methods with `NS_ENUM` ``` - (CGFloat)paddingForEdge:(YGEdge)edge; - (void)setPadding:(CGFloat)padding forEdge:(YGEdge)edge; ``` ## Usage in Objective-C ``` [view.layout setPadding:15 forEdge:YGEdgeAll]; // OR [view.layout setPadding:10 forEdge:YGEdgeTop]; // OR // NO WAY TO SET START [view.layout setPadding:10 forEdge:YKEdgeHorizontal]; [view.layout setPadding:10 forEdge:YKEdgeTop]; ``` ## Usage in Swift ``` view.layout.setPadding(15, for: .all) // OR view.layout.setPadding(10, for: .top) // OR // NO WAY TO SET START // OR view.layout.setPadding(10, for: .horizontal) view.layout.setPadding(10, for: .top) ``` ## Pros * The usage looks cleaner by having a smaller API surface area. ## Cons * We can't use dot-notation like all other properties, which breaks the reading flow. Perhaps it's my personal taste, but I see that as a big minus. ``` root.layout.width = 5; [root.layout setPadding:10 forEdge:YGEdgeTop]; root.layout.flexGrow = 1; ``` # `YGEdgeValue` ``` @interface YGEdgeValue @property (nonatomic) CGFloat left; @property (nonatomic) CGFloat right; @property (nonatomic) CGFloat top; @property (nonatomic) CGFloat bottom; @property (nonatomic) CGFloat start; @property (nonatomic) CGFloat end; @property (nonatomic) CGFloat horizontal; @property (nonatomic) CGFloat vertical; @property (nonatomic) CGFloat all; @end @inteface YGLayout @property (nonatomic, readonly) YGEdgeValue *padding; @end ``` ## Usage in Objective-C ``` view.layout.padding.all = 15; // OR view.layout.padding.top = 10; // OR view.layout.padding.start = 10; // OR view.layout.padding.horizontal = 10; view.layout.padding.top = 15; ``` ## Usage in Swift ``` view.layout.padding.all = 15 // OR view.layout.padding.top = 10 // OR view.layout.padding.start = 10 // OR view.layout.padding.horizontal = 10 view.layout.padding.top = 10 ``` ## Pros and Cons This is basically a different flavor of properties. I'm not a hug fan of it because it multiplies the number of objects on the heap that yoga generates by 4 for a small gain in readability and API surface and a small loss in usage typing speed because having to go through two auto-completion hints (`padding`, then `top`). # Conclusion On and all, here's my ranking of preference: 1. Properties 2. YGEdgeValue 3. Methods

hartbit (Migrated from github.com) reviewed 2016-12-20 16:34:24 -08:00

YogaKit/YKLayout.h

						
				@@ -0,0 +40,4 @@

				@property (nonatomic) CGFloat positionBottom;

				@property (nonatomic) CGFloat positionStart;

				@property (nonatomic) CGFloat positionEnd;

				@property (nonatomic) CGFloat positionHorizontal;

hartbit (Migrated from github.com) commented

2016-12-20 16:34:24 -08:00

As an additional support for the properties, I don't think it would surprise UIKit users because it is fairly idiomatic of the AutoLayout APIs:

Add to that the fact that CSS and React Native users would feel right a home.

As an additional support for the properties, I don't think it would surprise UIKit users because it is fairly idiomatic of the AutoLayout APIs: <img width="368" alt="screen shot 2016-12-21 at 01 31 09" src="https://cloud.githubusercontent.com/assets/325519/21373209/5f647db0-c71d-11e6-8cac-5853287cbeec.png"> Add to that the fact that CSS and React Native users would feel right a home.

emilsjolander commented

2016-12-21 07:26:15 -08:00

(Migrated from github.com)

@hartbit I talked to @dshahidehpour offline, we are both super excited and happy with the direction of this.

~~We prefer YGEdgeValue approach as is makes the header for YogaKit much easier to understand as there will be a lot less properties to parse.~~

So what we see as left to do here is:

Split out what is possible into separate smaller pull requests (thanks for taking the time to do this already, makes it much easier for us)
rename layout property yoga
rename apply to applyLayout
revert YK prefix back to YG
@property (nonatomic, readwrite, assign) ...

@hartbit I talked to @dshahidehpour offline, we are both super excited and happy with the direction of this. <del>We prefer `YGEdgeValue` approach as is makes the header for YogaKit much easier to understand as there will be a lot less properties to parse.</del> So what we see as left to do here is: - Split out what is possible into separate smaller pull requests (thanks for taking the time to do this already, makes it much easier for us) - rename `layout` property `yoga` - rename `apply` to `applyLayout` - revert `YK` prefix back to `YG` - `@property (nonatomic, readwrite, assign) ...`

d16r (Migrated from github.com) reviewed 2016-12-22 06:20:39 -08:00

YogaKit/YKLayout.h

						
				@@ -0,0 +40,4 @@

				@property (nonatomic) CGFloat positionBottom;

				@property (nonatomic) CGFloat positionStart;

				@property (nonatomic) CGFloat positionEnd;

				@property (nonatomic) CGFloat positionHorizontal;

d16r (Migrated from github.com) commented

2016-12-22 06:20:39 -08:00

Hey @hartbit, sorry I took so long to respond. I really appreciate the time you put into that response, and I support all of it 100%.

@emilsjolander I know we talked about YGEdgeValue, but, I think @hartbit makes a compelling case for why we should put these properties on the layout object.

Matches other UIKit APIs.
Less objects on the heap.
One less method lookup when we are getting/setting this on the objc runtime.

Hey @hartbit, sorry I took so long to respond. I really appreciate the time you put into that response, and I support all of it 100%. @emilsjolander I know we talked about `YGEdgeValue`, but, I think @hartbit makes a compelling case for why we should put these properties on the layout object. 1. Matches other UIKit APIs. 2. Less objects on the heap. 3. One less method lookup when we are getting/setting this on the objc runtime.

d16r commented

2016-12-22 06:35:22 -08:00

(Migrated from github.com)

@hartbit Thanks for being patient! I've talked @emilsjolander and we've decided to follow your argument and not use YGEdgeValue. I'm removed it from the list of requested fixes above, thanks!

@hartbit Thanks for being patient! I've talked @emilsjolander and we've decided to follow your argument and not use `YGEdgeValue`. I'm removed it from the list of requested fixes above, thanks!

hartbit commented

2016-12-26 08:27:12 -08:00

(Migrated from github.com)

@emilsjolander @dshahidehpour I started by creating a pull request for an Xcode framework target to get it working with Carthage. Unfortunately, I've hit a few road blocks when doing that. Care to have a look? #305

hartbit commented

2016-12-26 08:32:28 -08:00

(Migrated from github.com)

By the way, while looking at the changes necessary, I saw that the yoga property still points to a YGLayout instance. For the same reasons that you did not like the layout name, I should probably rename that class to YGYoga. But while thinking about it, I thought: why not rename both to style and YGStyle?

override func viewDidLoad() {
    let root = view!
    root.backgroundColor = .red
    root.style.isEnabled = true
    root.style.isIncludedInLayout = true
    root.style.width = view.bounds.size.width
    root.style.height = view.bounds.size.height
    root.style.alignItems = .center
    root.style.justifyContent = .center
    root.style.paddingLeft = 5
    root.style.marginAll = 10
    root.style.applyLayout()
}

By the way, while looking at the changes necessary, I saw that the `yoga` property still points to a `YGLayout` instance. For the same reasons that you did not like the `layout` name, I should probably rename that class to `YGYoga`. But while thinking about it, I thought: why not rename both to `style` and `YGStyle`? ``` override func viewDidLoad() { let root = view! root.backgroundColor = .red root.style.isEnabled = true root.style.isIncludedInLayout = true root.style.width = view.bounds.size.width root.style.height = view.bounds.size.height root.style.alignItems = .center root.style.justifyContent = .center root.style.paddingLeft = 5 root.style.marginAll = 10 root.style.applyLayout() } ```

d16r commented

2016-12-26 08:38:43 -08:00

(Migrated from github.com)

@hartbit I'll be back online tomorrow, and I'll be able to take a look!

hartbit commented

2016-12-27 06:08:54 -08:00

(Migrated from github.com)

Scratch my last message. The fact that the object also has "layout" properties like instrinsictSize makes me reconsider my thought. So, should we rename it to YGYoga? Sounds a bit weird.

Scratch my last message. The fact that the object also has "layout" properties like `instrinsictSize` makes me reconsider my thought. So, should we rename it to `YGYoga`? Sounds a bit weird.

emilsjolander commented

2016-12-28 03:58:05 -08:00

(Migrated from github.com)

@hartbit I think YGLayout is fine. The reason i was against naming the property layout is the same reason I would be against style, they are very generic name for properties in a UIView category. Let's stick with yoga as the property name and YGLayout as the class name for now. This might change later but I would rather get this merged and then continue iterating on it.

@hartbit I think `YGLayout` is fine. The reason i was against naming the property `layout` is the same reason I would be against `style`, they are very generic name for properties in a UIView category. Let's stick with `yoga` as the property name and `YGLayout` as the class name for now. This might change later but I would rather get this merged and then continue iterating on it.

👍 1 ❤️ 1

eklipse2k8 (Migrated from github.com) reviewed 2017-01-03 11:37:41 -08:00

YogaKit/YKLayout.h

						
				@@ -0,0 +40,4 @@

				@property (nonatomic) CGFloat positionBottom;

				@property (nonatomic) CGFloat positionStart;

				@property (nonatomic) CGFloat positionEnd;

				@property (nonatomic) CGFloat positionHorizontal;

eklipse2k8 (Migrated from github.com) commented

2017-01-03 11:37:41 -08:00

I like the YGEdgeValue the best, but why does it need to be a full class? In C/C++, the pattern would be lightweight structs for this kind of data set. That way I can do something like:

view.layout.padding = {.top = 10, .bottom = 10}

Or a hypothetical
view.layout.padding = YGCreateEdgeValues(...);

I like the YGEdgeValue the best, but why does it need to be a full class? In C/C++, the pattern would be lightweight structs for this kind of data set. That way I can do something like: `view.layout.padding = {.top = 10, .bottom = 10}` Or a hypothetical `view.layout.padding = YGCreateEdgeValues(...);`

eklipse2k8 commented

2017-01-03 11:50:26 -08:00

(Migrated from github.com)

@emilsjolander @dshahidehpour what was the reason for rejecting YGEdgeValue? If it's represented as a lightweight struct, I think that's more idiomatic UIKit.

I commented above not seeing your strikethrough, but I'll repost it here as I feel strongly this is very easy to read and you can pass the struct around.
view.layout.padding = {.top = 10, .bottom = 10}

or even create once and set to multiple views instead of setting properties.

@emilsjolander @dshahidehpour what was the reason for rejecting YGEdgeValue? If it's represented as a lightweight struct, I think that's more idiomatic UIKit. I commented above not seeing your strikethrough, but I'll repost it here as I feel strongly this is very easy to read and you can pass the struct around. `view.layout.padding = {.top = 10, .bottom = 10}` or even create once and set to multiple views instead of setting properties.

d16r commented

2017-01-03 12:14:30 -08:00

(Migrated from github.com)

@eklipse2k8 We mentioned the reasons we preferred to not use it above:

Matches other UIKit APIs.

Less objects on the heap.

One less method lookup when we are getting/setting this on the objc runtime.

I went back and forth on this myself. I think what's best is for us to fixup/land this revision, and then iterate on refinements to the API afterwards. I'd be happy to talk to you more about this offline.

@eklipse2k8 We mentioned the reasons we preferred to not use it above: > 1. Matches other UIKit APIs. > 2. Less objects on the heap. > 3. One less method lookup when we are getting/setting this on the objc runtime. I went back and forth on this myself. I think what's best is for us to fixup/land this revision, and then iterate on refinements to the API afterwards. I'd be happy to talk to you more about this offline.

hartbit commented

2017-01-03 14:09:04 -08:00

(Migrated from github.com)

@emilsjolander @dshahidehpour what was the reason for rejecting YGEdgeValue? If it's represented as a lightweight struct, I think that's more idiomatic UIKit.

I understand what you mean. And I'm totally open to discussing it. But some problems remain:

It's not 100% idiomatic UIKit. Very little Objective-C code I've seen in the wild uses the {.top = 10, .bottom = 10} C syntax, and instead prefer CGRectMake, UIEdgeInsetsMake and other function initialisers. I was actually reminded of that syntax while looking into the Yoga code: I had completely forgotten about it while working several years in Objective-C land.
It does force an extra dot accessor when used from Swift, which does not have an equivalent C syntax:

view.yoga.padding.top = 10
view.yoga.padding.bottom = 10

I still think that having the exact same API as React Native to be a big plus.

> @emilsjolander @dshahidehpour what was the reason for rejecting YGEdgeValue? If it's represented as a lightweight struct, I think that's more idiomatic UIKit. I understand what you mean. And I'm totally open to discussing it. But some problems remain: 1. It's not 100% idiomatic UIKit. Very little Objective-C code I've seen in the wild uses the `{.top = 10, .bottom = 10}` C syntax, and instead prefer `CGRectMake`, `UIEdgeInsetsMake` and other function initialisers. I was actually reminded of that syntax while looking into the Yoga code: I had completely forgotten about it while working several years in Objective-C land. 2. It does force an extra dot accessor when used from Swift, which does not have an equivalent C syntax: ``` view.yoga.padding.top = 10 view.yoga.padding.bottom = 10 ``` 3. I still think that having the exact same API as React Native to be a big plus.

hartbit commented

2017-01-03 14:11:37 -08:00

(Migrated from github.com)

By the way, I was on vacation until now, but I will be working on getting this PR back into shape very soon.

👍 2

emilsjolander commented

2017-01-05 08:15:22 -08:00

(Migrated from github.com)

@hartbit What is the ETA on this? Would love to get this in before https://github.com/facebook/yoga/pull/278 and https://github.com/facebook/yoga/pull/280

hartbit commented

2017-01-05 08:20:58 -08:00

(Migrated from github.com)

Was planning on getting it done this evening.

On 5 Jan 2017, at 17:15, Emil Sjölander notifications@github.com wrote:

@hartbit https://github.com/hartbit What is the ETA on this? Would love to get this in before #278 https://github.com/facebook/yoga/pull/278 and #280 https://github.com/facebook/yoga/pull/280
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/facebook/yoga/pull/289#issuecomment-270683912, or mute the thread https://github.com/notifications/unsubscribe-auth/AAT3j4Pc7xpq4k5bEtJZmX55hzk9BtTMks5rPRcggaJpZM4LQPXG.

Was planning on getting it done this evening. > On 5 Jan 2017, at 17:15, Emil Sjölander <notifications@github.com> wrote: > > @hartbit <https://github.com/hartbit> What is the ETA on this? Would love to get this in before #278 <https://github.com/facebook/yoga/pull/278> and #280 <https://github.com/facebook/yoga/pull/280> > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub <https://github.com/facebook/yoga/pull/289#issuecomment-270683912>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAT3j4Pc7xpq4k5bEtJZmX55hzk9BtTMks5rPRcggaJpZM4LQPXG>. >

👍 1

hartbit commented

2017-01-05 15:24:50 -08:00

(Migrated from github.com)

I did it in a separate PR to make it easier for me to work from the latest commit of master: #322

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#289

WIP: Improve the Objective-C and Swift bridge #289

What I've done

Objective-C

Swift

YKEdge based style values

Normalization

Sub-types

Prefix in Swift

Objective-C

Swift

Post Scriptum

Properties

Usage in Objective-C

Usage in Swift

Pros

Cons

Methods with NS_ENUM

Usage in Objective-C

Usage in Swift

Pros

Cons

YGEdgeValue

Usage in Objective-C

Usage in Swift

Pros and Cons

Conclusion

Pull request closed

Methods with `NS_ENUM`

`YGEdgeValue`