Make textContainerInset configurable for ASEditableTextNode placehold…

[samhsiung]

Make textContainerInset configurable for ASEditableTextNode placeholder and typed textViews

Previously, it was only possible to configure the textContainerInset of the typed textView by accessing the textView property on didLoad. This would cause the placeholder and typed textViews to become desynced however, so in this commit we add the ability to configure both.

[facebook-github-bot]

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!

[levi]

The value of the new property will only be recognized on first load. I would recommend extending setTextContainerInset: to support value changes after the view has loaded.

[samhsiung]

Nice catch, added setter logic and tested that it works

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[appleguy]

Would accessing textView here cause the view to be created? Naturally that would be bad...it may be best to wrap the last two lines in a check of [self isNodeLoaded](I may have the exact property name wrong)

[samhsiung]

Thanks for the review. Would that be the case if we were accessing self.textView. In this case we're accessing the textView property from _textKitComponents which seems to be generated on self.textView instead.

Also I just accidentally nuked my master branch so I'm going to try to reopen this :)

[appleguy]

Thanks @samhsiung, this is a great extension to a lesser-used class. It is awesome to have some attention applied to these classes because it really helps round out the framework and provide a more confident developer experience when they are needed.

[samhsiung]

Moved this PR here: #802

[JS1010111]

@samhsiung, I need textContainerInset configurable for ASTextNode but I can barely read obj-c since I'm learning Swift. Would you kindly make this change for ASTextNode also?

[samhsiung]

@jospalding have you tried using ASInsetLayoutSpec to wrap your text node to give it padding?

[JS1010111]

@samhsiung yes, ASInsetLayoutSpec partially solves my issue but my node has a backgroundColor and the inset does not respect it (not sure if this is expected). It adds the inset but the inset is transparent.
I did quite a lot of research and imho the most elegant way of doing what I need is using textContainerInset.
Thank you and please let me know if you have any other suggestion that I should try.

[samhsiung]

@jospalding have you tried setting the background color of the parent node that has the inset instead of the ASTextNode?

[JS1010111]

@samhsiung I can't set a background for the parent because it is also the parent of other nodes.
This is my layoutSpecThatFits for the parent (it's an ASCellNode):

override func layoutSpecThatFits(constrainedSize: ASSizeRange) -> ASLayoutSpec {
        return ASInsetLayoutSpec(
            insets: UIEdgeInsetsMake(10, 20, 30, 20),
            child: ASStackLayoutSpec(
                direction: .Vertical,
                spacing: 5,
                justifyContent: .Start,
                alignItems: .Start,
                children: [
                    channelNode,
                    titleNode,
                    ASRatioLayoutSpec(
                        ratio: 2/3,
                        child: imageNode
                    )
                ]
            )
        )
    }

And this is how it renders:

I need the channelNode to have the padding/inset. I could wrap it inside another ASDisplayNode but this looks like a workaround for me.

[samhsiung]

@jospalding

For now you can subclass ASTextNode and override - (id<ASLayoutable>)finalLayoutable to add the inset. I could work on adding an inset property to ASTextNode that does the same:

- (id<ASLayoutable>)finalLayoutable 
{
    ASInsetLayoutSpec *finalLayoutable = [[ASInsetLayoutSpec alloc] init];
    finalLayoutable.insets = UIEdgeInsetsMake(....);
    finalLayoutable.isFinalLayoutable = YES;

    // isFinalLayoutable must be set before calling setChild:. setChild: will ask the child
    // for its finalLayoutable which will cause infinte recursion.
    [finalLayoutable setChild:self];
    return finalLayoutable;
}

[samhsiung]

@jospalding could you create a separate issue for adding an inset property to ASTextNode for this so others in the community could add their thoughts?

[JS1010111]

@samhsiung nice, I will try to subclass it! I will open the issue and let you know! Thank you very much!

[JS1010111]

@samhsiung the finalLayoutable suggestion worked but does not ended up with what I was trying to achieve. Actually, the result was the same as wrapping the ASTextNode inside an ASInsetLayoutSpec, adding "margins" (as in CSS) to the ASTextNode.

I ended up with something relatively simple, but I'm still not sure if it's the best soluiton.
Please take a look at #1138 and give your thoughts.

Thanks again!