[ASCornerLayoutSpec] New layout spec class for declarative corner element layout. #657

huang-kun · 2017-11-02T13:28:45Z

This PR includes implementation code, snapshot test and examples (both objc and swift).

huang-kun · 2017-11-02T13:32:44Z

This PR comes from #623. I will start one commit here instead of a whole mass. Make it easy for code review and CI build.

…CHANGELOG.md

nguyenhuy

Thanks for working on this. There are a few things we should address before merging.

On a separate note, and definitely not a blocker for this PR, but it would be great if we can update the README file in Examples dir to include some screenshots of your additions. That would help people to quickly understand your new spec and nodes without running the sample.

nguyenhuy · 2017-11-21T12:22:57Z

Source/Layout/ASCornerLayoutSpec.h

+@property (nonatomic, assign) CGPoint offset;
+
+/**
+ Should include corner element into layout size calculation. If included, the layout size will be the union size of both child and corner; If not included, the layout size will be only child's size. Default is NO.


Nit: This line is too long. Please split it into multiple ones, maximum 120 chars each. Same for some other lines each this file.

nguyenhuy · 2017-11-21T12:29:05Z

Source/Layout/ASCornerLayoutSpec.h

+/**
+ Should include corner element into layout size calculation. If included, the layout size will be the union size of both child and corner; If not included, the layout size will be only child's size. Default is NO.
+ */
+@property (nonatomic, assign) BOOL includeCornerForSizeCalculation;


How about wrapsCorner?

nguyenhuy · 2017-11-21T12:31:35Z

Source/Layout/ASCornerLayoutSpec.h

+
+NS_ASSUME_NONNULL_BEGIN
+
+typedef NS_ENUM(NSInteger, ASCornerLayoutLocation) {


Coding convention: Method, @interface, and @implementation brackets on the following line.

Please also update the rest of this diff to follow the convention.

nguyenhuy · 2017-11-21T12:37:12Z

Source/Layout/ASCornerLayoutSpec.mm

+  CGSize size = element.style.preferredSize;
+  if (!CGSizeEqualToSize(size, CGSizeZero) && !ASIsCGSizeValidForSize(size) && (size.width < 0 || (size.height < 0))) {
+    NSString *failedReason = [NSString stringWithFormat:@"[%@]: Should give a valid preferredSize value for %@ before corner's position calculation.", self.class, element];
+    @throw [NSException exceptionWithName:NSInternalInconsistencyException reason:failedReason userInfo:nil];


Let's be lenient by triggering assertions instead of throwing exceptions in development, and fall back to a default size/layout in production. What do you think?

nguyenhuy · 2017-11-21T12:52:26Z

Source/Layout/ASCornerLayoutSpec.mm

+  [self validateElement:corner];
+
+  // Layout child
+  ASLayout *childLayout = [child layoutThatFits:constrainedSize parentSize:constrainedSize.max];


When the constrained size range has an infinite max size, the parent size needs to be adjusted. Please either override calculateLayoutThatFits:restrictedToSize:relativeToParentSize:, or adjust the size like this.

nguyenhuy · 2017-11-21T12:58:50Z

Source/Layout/ASCornerLayoutSpec.mm

+
+  // Update corner's position
+  cornerLayout.position = relativePosition;
+  corner.style.layoutPosition = relativePosition;


I don't think this is needed.

nguyenhuy · 2017-11-21T13:08:00Z

Source/Layout/ASCornerLayoutSpec.mm

+  CGRect frame = childLayout.frame;
+  if (_includeCornerForSizeCalculation) {
+    frame = CGRectUnion(childLayout.frame, cornerLayout.frame);
+    frame.size = ASSizeRangeClamp(constrainedSize, frame.size);


Yes, always clamp against the constrained size range 👍

nguyenhuy · 2017-11-21T13:09:43Z

Tests/ASCornerLayoutSpecSnapshotTests.mm

+
+- (void)testCornerSpecWithLocation:(ASCornerLayoutLocation)location
+                      offsetOption:(ASCornerLayoutSpecSnapshotTestsOffsetOption)offsetOption
+                     childSizeOnly:(BOOL)childSizeOnly {


Let's use the same name that the corner spec uses (includeCornerForSizeCalculation or wrapsCorner depends on its final name).

nguyenhuy · 2017-11-21T13:19:52Z

examples/LayoutSpecExamples-Swift/Sample/LayoutExampleNode.swift

+  private func circularize(_ node: ASDisplayNode, in size: CGSize) {
+    assert(size.width == size.height)
+    node.style.preferredSize = size
+    node.cornerRadius = size.width / 2


Not a blocker and don't spend too much time on this, but please consider other rounding techniques mentioned here, if possible. I'm not concerned about the performance in this sample application, but rather using best practices in example code.

nguyenhuy · 2017-11-21T13:23:05Z

examples/LayoutSpecExamples/Sample/LayoutExampleNodes.m

+        _badgeNode = [ASTextNode new];
+        _badgeNode.attributedText = numberText;
+        _badgeNode.backgroundColor = UIColor.redColor;
+        _badgeNode.cornerRadius = 12;


Same here and below, let's avoid cornerRadius whenever makes sense.

nguyenhuy

I can't be happier to hit the approve button! Thank you for not only putting up a new layout spec but also taking care of tests and examples. Way to go for all of us!

nguyenhuy · 2017-11-22T11:19:00Z

Source/Layout/ASCornerLayoutSpec.mm

+#import <AsyncDisplayKit/ASLayoutSpec+Subclasses.h>
+#import <AsyncDisplayKit/ASDisplayNode.h>
+
+CGPoint as_calculatedCornerOriginIn(CGRect baseFrame, CGSize cornerSize, ASCornerLayoutLocation cornerLocation, CGPoint offset) {


Nit: open curly bracket in a new line.

Thank you for refining so much details on the new layout spec. I kept the curly bracket at the end of function as the code guide line declared. 🤔🤔🤔 It says:

Function brackets on the same line

static void someFunction() { // Implementation }

nguyenhuy · 2017-11-22T11:20:10Z

Tests/ASCornerLayoutSpecSnapshotTests.mm

+
+@implementation ASCornerLayoutSpecSnapshotTests
+
+- (void)setUp {


Nit: method curly brackets in new lines throughout this file.

nguyenhuy · 2017-11-22T11:21:57Z

Source/Layout/ASCornerLayoutSpec.mm

+
+#pragma mark - Calculation
+
+- (ASLayout *)calculateLayoutThatFits:(ASSizeRange)constrainedSize


This whole layout algorithm is a beauty!

Great to hear that 😁

maicki

@huang-kun Thank you, pretty great work on the ASCornerLayoutSpec!

nguyenhuy · 2017-11-24T13:45:51Z

Here we go. Thanks again!

huang-kun · 2017-11-25T02:20:19Z

Would you like me to introduce the ASCornerLayoutSpec into your official doc at here after new version release? @nguyenhuy @maicki

nguyenhuy · 2017-11-25T08:03:24Z

Yes, that would be great!

…

-- Best regards, Huy Nguyen

On November 25, 2017 at 2:20:20 AM, huang-kun ***@***.***) wrote: Would you like me to introduce the ASCornerLayoutSpec into your official doc at here <https://github.com/TextureGroup/Texture/blob/master/docs/_docs/layout2-layoutspec-types.md> after new version release? @nguyenhuy <https://github.com/nguyenhuy> @maicki <https://github.com/maicki> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#657 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAj4Yp1NNNxeCdSm-2yxJI6tVEimUrZiks5s53lkgaJpZM4QPrn8> .

…ment layout. (TextureGroup#657) * Add new layout spec class with snapshot testing. Update examples and CHANGELOG.md * Code review updates. * Open curly bracket in a new line.

huang-kun added 2 commits November 2, 2017 20:50

Merge remote-tracking branch 'TextureGroup/master'

d4f358f

Merge remote-tracking branch 'TextureGroup/master'

043a2c5

Add new layout spec class with snapshot testing. Update examples and …

6fe4524

…CHANGELOG.md

huang-kun changed the title ~~[ASCornerLayoutSpec] new layout spec class for declarative corner element layout.~~ [ASCornerLayoutSpec] New layout spec class for declarative corner element layout. Nov 3, 2017

nguyenhuy requested changes Nov 21, 2017

View reviewed changes

Code review updates.

66df920

nguyenhuy approved these changes Nov 22, 2017

View reviewed changes

Open curly bracket in a new line.

69d941d

maicki approved these changes Nov 23, 2017

View reviewed changes

nguyenhuy added 2 commits November 23, 2017 18:01

Merge branch 'master' into master

1c441ae

Merge branch 'master' into master

4db2e07

TextureGroup deleted a comment Nov 23, 2017

nguyenhuy merged commit e4b2c05 into TextureGroup:master Nov 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASCornerLayoutSpec] New layout spec class for declarative corner element layout. #657

[ASCornerLayoutSpec] New layout spec class for declarative corner element layout. #657

huang-kun commented Nov 2, 2017

huang-kun commented Nov 2, 2017

nguyenhuy left a comment

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy Nov 21, 2017

nguyenhuy left a comment

nguyenhuy Nov 22, 2017

huang-kun Nov 23, 2017

nguyenhuy Nov 22, 2017

nguyenhuy Nov 22, 2017

huang-kun Nov 23, 2017

maicki left a comment

nguyenhuy commented Nov 24, 2017

huang-kun commented Nov 25, 2017

nguyenhuy commented Nov 25, 2017 via email


		NS_ASSUME_NONNULL_BEGIN

		typedef NS_ENUM(NSInteger, ASCornerLayoutLocation) {


		@implementation ASCornerLayoutSpecSnapshotTests

		- (void)setUp {


		#pragma mark - Calculation

		- (ASLayout *)calculateLayoutThatFits:(ASSizeRange)constrainedSize

[ASCornerLayoutSpec] New layout spec class for declarative corner element layout. #657

[ASCornerLayoutSpec] New layout spec class for declarative corner element layout. #657

Conversation

huang-kun commented Nov 2, 2017

huang-kun commented Nov 2, 2017

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maicki left a comment

Choose a reason for hiding this comment

nguyenhuy commented Nov 24, 2017

huang-kun commented Nov 25, 2017

nguyenhuy commented Nov 25, 2017 via email