Open garrettmoon opened 7 years ago
From @cesteban on April 3, 2017 23:44
In case it helps, I've created a public repo with these categories as they are now, and a brief README explaining the fluent API:
https://github.com/cesteban/ASDKFluentExtensions
It is not finished work, but it should be enough to transmit the idea. Any feedback would be very much appreciated.
Thanks!
From @MarvinNazari on April 5, 2017 7:25
@Adlai-Holler made something like this too https://github.com/Adlai-Holler/AsyncDisplayKitExtensions/blob/master/AsyncDisplayKitExtensions/Layout.swift It's not updated though.
From @cesteban on April 5, 2017 20:41
I didn't know about these extensions by @Adlai-Holler! It is the same concept, and even the syntax is almost identical. I was looking for feedback, so this is great, thanks a lot @MarvinNazari.
The implementation looks very similar to my first attempt on this. Extending ASLayoutElement
is much better because there is no code duplication. However protocol extension won't be visible from ObjC, so I abandoned that approach in favour of an ObjC implementation.
From @cesteban on April 5, 2017 22:10
That's great to know @MarvinNazari! I will create a public pod to make things easier. Any further comments you have please let me know.
@cesteban this is really neat and makes things far more readable. I really want @nguyenhuy and @maicki to weigh in as they're the layout experts.
From @cesteban on April 24, 2017 21:57
Thanks a lot @garrettmoon. Please let me know if I can do something in the meanwhile. I would be happy to help pushing this forward.
From @maicki on April 25, 2017 15:8
@cesteban Hey, I like the idea and as you can see in the ASDKGram
example I tried to design the current API with that in mind.
That said I don't think we should add this to the core framework though, but we would be very happy if you could bootstrap that in a separate repo / component and we can link to it and if other developer would like to use it can just add it via Cocoapods, Carthage etc.
Thoughts?
From @nguyenhuy on April 25, 2017 15:50
@cesteban First of all, thanks for sharing this awesome proposal! I like the syntax quite a lot.
However, having yet another way to construct layouts will be confusing to new folks and make it hard to maintain the documentation. Unless there is an overwhelming demand and support for this new syntax, we might not want to merge it to the core framework. And even if we do, it will need to replace the existing API which requires careful planning. The replacement deserves a major release on its own and with it updates in the documentation, examples, etc
For the time being, would you like us to help promote your work, @cesteban? We can have a page dedicated to 3rd-party extensions/integrations. IMHO, we can even host your repo under TextureGroup org (only if you want and with you being the primary author of course!).
From @cesteban on April 26, 2017 8:16
@maicki @nguyenhuy Thank you very much for your responses. I agree: Let's distribute this as a separated component and see if it gets some traction.
It would be great if you help promoting these extensions though. I would be happy as well having the repo under TextureGroup, if you think that's ok. I'm not specially worried about authorship, I just want this to be more widespread. It's been very useful to me when writing Texture layout code, but it doesn't make much sense if I'm the only one using it 😅
Thank you @garrettmoon for moving this issue.
@maicki @nguyenhuy past week I didn't had time to work on this. This week I will brush up the current repo. I want to add more documentation. I was thinking on creating a clone of ASDKgram using this fluent API. That way people wanting to approach to this new syntax will have a familiar example to look at. I also want to add Carthage support and clean up a little bit.
I'm open to suggestions on what to put the focus on.
@cesteban let us know when you think this is production ready, we'd love to give it a try over here at Pinterest!
@garrettmoon @maicki @nguyenhuy Thanks a lot for your patience, very busy week.
I updated the repo of ASDKFluentExtensions with a lot of documentation and sample code. I also documented the code a bit, and added some missing pieces.
I also created a clone of ASDKgram adapting the layout code to this new fluent style. Then I kept going and ported 8 examples more, just to make sure the component was ready, and for documentation purposes. I have the examples in a fork of Texture so I can keep them up to date with your changes. You can see the list of ported examples in the README, or you can have a look on all the changes together here. I know you guys have been talking on refactoring the examples. It would be very easy to port the final combined example when the day comes, I'll keep an eye on it.
I couldn't add Carthage support because currently ASDKFluentExtensions uses Cocoapods to fetch Texture. I can move the project itself to Carthage to solve this (it will take a few minutes), or if you have a better idea please let me know (I don't use Carthage myself, so not an expert here 😅).
Edit: I finally added Carthage support as well, starting from version 0.6. Forget the previous paragraph.
I believe this is basically production ready. Of course I'll keep updating the repo, but for a first version it should be enough. Please let me know your thoughts. Any feedback you have is very valuable to me.
Thank you!
@garrettmoon Any updates on this?
@cesteban sorry, sorry! We've been super busy, I'll see if I can't get it into the hands of those that would benefit most in the next couple weeks.
@garrettmoon Thank you very much. Let me know if I can do something to push this.
Hi @cesteban
I was trying the Fluent layout specs last week and I think they are really well thought and easy to use. Definitely it gives a different perspective when writing layouts with Texture. I think it works great with simple and static layouts as the code better represents (visually) what is being laid-out. Really awesome!
That being said, I believe there are a few challenges that I faced when using it.
withChildren:
is defined in a category on ASStackLayoutSpec
. This creates a limitation that only ASStackLayoutSpec
s instances can use this convenience method. If this could be moved higher up in the hierarchy, more classes would be benefit from it. If you pay attention, the children
property is defined in ASLayoutSpec
, so maybe it would make more sense to have a category on that instead, so all layout specs (including custom subclasses) could have support for this as well. I believe this is an easy problem to solve.The tests that I've done are in ObjC. I believe there is more potential for this in Swift though as the code can become much more compact and easier to read, even with more complexes layout. Disclosure: I'm not a Swift expert.
@rmls Thanks a lot for your feedback! Very much appreciated.
I'm glad you're finding the fluent extensions interesting. I'll try to respond to your comments the best way I can.
It would be very easy to move withChildren:
to ASLayoutSpec
. I never thought it could be interesting to use it outside of a ASStackLayoutSpec
, but of course it makes a lot of sense to have this convenience available in custom layout specs. I'll do the change and see how it looks. Thank you very much for the suggestion, amazing feedback.
It is true that in ObjC, when you have very complex layouts with conditionals and branches, lines could get very long, compromising readability. There is a good example of this in the version of ASMapNode that I wrote to demonstrate fluent layout. Unfortunately, the syntax of ObjC makes it really challenging to use more functional idioms (for example this is a limitation I faced with Objective-C versions of ReactiveCocoa, a framework that also relays heavily on method chaining). In these cases, you can always split the layout in sorter, meaningful pieces, but as you said, the advantages of fluent style get diluted if you do so. To me, the best solution in these cases is always to try to make smaller pieces of layout. When I find my layoutSpecThatFits:
overcrowded, I take that as a signal that the node should be decomposed in smaller nodes with their own layout. This way they can be more easily reused, and separately maintained. But of course that's only my way of working, and it doesn't apply to everybody.
Also, as you said, this problem basically doesn't exist in Swift. As opposed to Objective-C, Swift is extremely suited for these type of interfaces. But that's a different story.
I'm sorry I don't have a better answer for you! 😅 Any suggestion you can give on how to mitigate this limitation would be extremely appreciated. Also, feel free to send pull requests or open issues in the ASDKFluentExtensions repo. It would be so great to have more people involved!
From @cesteban on April 2, 2017 23:50
I've been using AsyncDisplayKit for a while. It is a beautifully designed API, thanks a lot for your work. However, I've been missing a more visual interface for defining layouts. I created a bunch of categories over
ASDisplayNode
andASLayoutElement
that make it possible to write layout code using a fluent API. So for example the layout ofPhotoTableNodeCell
in ASDKgram-Swift example can be translated to this:This layout reads from top to bottom without interruptions. The outermost code define the general structure, and inner parts define the details, so less visual scanning needed. Also, it is much faster to add, remove, and reorganize specs. Finally, note that modifications of
style
properties are also composable with layout specs (see usages ofwithPreferredSize
orwithFlexShrink
inline with layout specs definition). This avoids interruptions on how the code visually flows.To me this is has been a great improvement in the way I write ASDK layout code. However, I would like to know whether this type of API fits the vision you have for how AsyncDisplayKit should be used. I'm relative newcomer to ASDK, so perhaps I'm walking in the wrong direction?
If there is interest in this categories I can brush them up and send a PR or perhaps distribute them as a separated component.
Copied from original issue: facebookarchive/AsyncDisplayKit#3238