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

Propagate Style Properties through Inset, Other Layout Specs #159

Open
garrettmoon opened this issue May 1, 2017 · 5 comments
Open

Propagate Style Properties through Inset, Other Layout Specs #159

garrettmoon opened this issue May 1, 2017 · 5 comments

Comments

@garrettmoon
Copy link
Member

From @Adlai-Holler on November 18, 2016 6:7

Users are often surprised that when they set properties like flexShrink on a node, and then put that node inside an inset spec, the value they set is not used.

It might be valuable to propagate these values outward in some cases. Some initial thoughts:

  1. We will need a way to know whether a value was set explicitly or is a default.
  2. Should we propagate style for all specs that have only one child – inset, ratio, center, relative?
  3. Should we propagate style from the "main" child in overlay and background specs?
  4. Would it make sense to remove the style properties from the affected specs, so that setting the style of the affected node is the way to configure the style? If we do this, then point 1 disappears.

See also:
facebookarchive/AsyncDisplayKit#2601
facebookarchive/AsyncDisplayKit#874

Let's gather thoughts @nguyenhuy @maicki @hannahmbanana @appleguy @lappp9 @garrettmoon

Copied from original issue: facebookarchive/AsyncDisplayKit#2628

@garrettmoon
Copy link
Member Author

From @lappp9 on November 18, 2016 16:55

So personally, when I'm using any of these one-child specs I tend to think of them almost as behaviors I'm adding to the node instead of really focusing on them as objects. Maybe other people feel the same way and that's where the confusion comes in?

I'm curious, is there an argument against having them propagate outwards? From my perspective it seems like it would only smooth the learning curve of the layout system?

(Along these same lines, and maybe not for a while, but couldn't some things like ASInsetSpec just be a UIEdgeInsets property called contentInset on the style object? Ratio could probably work that way too, though relative and center have a bit more going on.)

@garrettmoon
Copy link
Member Author

From @stephensilber on November 21, 2016 19:14

As someone who is still in the learning phase of ASDK, I will say that it was confusing to me that properties set on a node then needed to be applied to the layout spec containing that node. I understand this piece now, but if there are no benefits to just propagating changes outward, it would have saved me a lot of headaches in the beginning.

@garrettmoon
Copy link
Member Author

From @george-gw on November 22, 2016 8:33

Can someone describe an example use case where a single-child layout needs a different style than its child? I haven't come across such a case yet.

I'm not sure which is better:

  1. Removing the style object:
    which would make the affected layouts inconsistent with other layouts
  2. Linking the style objects of the layout with that of the node:
    which might give the impression (from a user's perspective) that you can actually set different styles.

In any case, I agree with @lappp9, I also think of those one-child specs as extensions to the styles basically. Although, I would still want some of them to be separate from the style object.

@garrettmoon
Copy link
Member Author

From @skensell on November 22, 2016 10:17

I think propagating flexShrink and flexGrow outward is a terrific idea. When debugging a layout I often find myself looking for the missing flexGrow assignment. And I have helper methods like this for stacks:

+ (ASStackLayoutSpec *)flexibleVerticalStackWithChildren:(NSArray<id<ASLayoutable>> *)children {
    ASStackLayoutSpec* stack = [ASStackLayoutSpec verticalStackLayoutSpec];
    stack.flexGrow = YES;
    stack.flexShrink = YES;
    for (id <ASLayoutable> child in children) {
        child.flexGrow = YES;
        child.flexShrink = YES;
    }
    [stack setChildren:children];
    return stack;
}

Personally, I think the default for ASTextNode should be YES. Granted, I haven't spent much time with the innards of ASDK.

@garrettmoon
Copy link
Member Author

From @hannahmbanana on November 22, 2016 18:6

@george-gw - I don't think we've come across a case where a single-child layout needs a different style than its child...yet.

One downside to automatic style propagation for just the single child specs is that it may cause confusion. With the pre-2.0 Layout API, there was quite a bit of confusion around the ASLayoutable properties - some of which only applied for ASStackLayoutables (children of stacks) and ASStaticLayoutables (children of static specs).

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

No branches or pull requests

1 participant