[ASLayoutSpec] Added new Relative position layout spec

[stowy]

@appleguy here is the new layout spec I wrote, based very much on the ASCenterLayoutSpec but with more versatility / functionality. Would be useful for example for my badge node in the cat sample, to position the badge in the bottom left or bottom right of the image, depending on RTL.

Haven't added any tests for it, not familiar with how they are implemented here.

[appleguy]

cc @rcancro, @nguyenhuy

[nguyenhuy]

How about ASRelativePositionLayoutSpecPosition**Start** and ASRelativePositionLayoutSpecPosition**End**. We have been using "start" and "end" for similar purposes, for example in ASStackLayoutJustifyContent, ASStackLayoutAlignItems, etc.

P/S: I've just realized that we should use the same convention for ASHorizontalAlignment and ASVerticalAlignment. Will make an issue/PR for it.

[nguyenhuy]

@stowy: Awesome spec that puts center spec to shame! I'm wondering about what we should do to center spec once this one is accepted. Should we deprecate/remove it? or make it a subclass of this one?

[nguyenhuy]

Regarding testing, we write snapshot test cases for layout specs. Check ASCenterLayoutSpecSnapshotTests out and let me know if you have any questions/problems. More than happy to help :)

Thanks a lot for contributing to ASDK 👍

[nguyenhuy]

On the name of this new spec, I think ASRelativeLayoutSpec is concise and good enough, no?

[stowy]

Thanks @nguyenhuy for the comments, will revise. I will checkout the snapshot tests also.

[stowy]

@nguyenhuy I've added the snapshot test cases, hopefully the build should pass and we're all set.

[nguyenhuy]

When first introduce the tests, you need to enable record mode (self.recordMode = YES;) and add the generated reference images to the repo. Later test runs will use these images to compare against new ones and fail if they don't match.

[nguyenhuy]

Please fix indentation.

[nguyenhuy]

Friendly reminder: please fix indentation.

[stowy]

@nguyenhuy, thanks for your help. In running tests in record mode, does that mean the tests need to run in record mode on every build machine? Is there more than one build machine? I ran in record mode once but I got the resource not found error after turning it off, would that be because the tests ran on a different machine? Or the record mode didn't run properly the first time?

[nguyenhuy]

Sorry for the confusion. First, on your local machine, you need to run those test cases with record mode enabled. This will generate a bunch of reference images. Turn off record mode, commit those images to the repo and push it. Once available, those images will be picked up to test against by other machines.

Here is an example of a PR that adds new snapshot test cases and reference images.

[stowy]

Oh right yes I had run them on my machine but didn't realise I had to
commit them to the repo.

Easy done, thanks.
On Sun, Jan 24, 2016 at 9:42 PM Huy Nguyen notifications@github.com wrote:

Sorry for the confusion. First, on your local machine, you need to run
those test cases with record mode enabled. This will generate a bunch of
reference images. Turn off record mode, commit those images to the repo and
push it. Once available, those images will be picked up to test against by
other machines.

Here https://github.com/facebook/AsyncDisplayKit/pull/848/files is an
example of a PR that adds new snapshot test cases and reference images.

—
Reply to this email directly or view it on GitHub
#1098 (comment)
.

[stowy]

@nguyenhuy I've added the images to the repo but seeing the following failure:

  0) -[ASRelativeLayoutSpecSnapshotTests testMinimumSizeRangeIsGivenToChildWhenNotPositioning] (AsyncDisplayKitTests.xctest)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
/Users/travis/build/facebook/AsyncDisplayKit/AsyncDisplayKitTests/ASLayoutSpecSnapshotTestsHelper.m:36: ((comparisonSuccess__) is true) failed - Snapshot comparison failed: Error Domain=FBSnapshotTestControllerErrorDomain Code=1 "Unable to load reference image." UserInfo={NSLocalizedFailureReason=Reference image not found. You need to run the test in record mode, NSLocalizedDescription=Unable to load reference image., FBReferenceImageFilePathKey=/Users/travis/build/facebook/AsyncDisplayKit/AsyncDisplayKitTests/ReferenceImages_32/ASRelativeLayoutSpecSnapshotTests/testMinimumSizeRangeIsGivenToChildWhenNotPositioning@2x.png}:

Any idea why it is looking under 'AsyncDisplayKitTests/ReferenceImages_32' instead of 'AsyncDisplayKitTests/ReferenceImages_64'? I don't see any other snapshot images in the _32 folder.

[nguyenhuy]

Nit: newline at the end of file

[stowy]

I've added the new line.

[nguyenhuy]

@stowy: Great. Nits aside, I think this PR is ready to land.

Regarding missing reference images for 32bit, I'm not entirely sure if we can force machines to always run tests on 64bit simulators, or ask it to look for images in 64bit directory. Maybe having the same set of images for both architectures is not a bad idea at all.

[stowy]

@nguyenhuy have addressed the last comments now I believe

[nguyenhuy]

Great. Looks good to me!

[stowy]

Thanks for your review and help @nguyenhuy, i enjoyed learning the snapshot tests process!

[appleguy]

@stowy @nguyenhuy thanks a lot for working together to refine this and make it production ready, especially including tests!

@Adlai-Holler, @RCacheaux, @lappp9, @rcancro, @levi what do you guys think we should do with the center layout spec? Keep it as is? Make it a special case built on top of this? Delete it entirely? Is there any more descriptive name for this class than Relative, especially because there is a relative type of dimension that doesn't relate to the alignment features of this class?

[Adlai-Holler]

This is great! I will put it to good use for sure!

Unless we can come up with a better name let's keep center layout spec and update its implementation to use this behind the scenes. My rationale is that it's a common use case and as a newcomer to the framework definitely would not think to look at ASRelativeLayoutSpec. I think the clarity is worth the slight redundancy.

[nguyenhuy]

@stowy Do you have time to fix conflicts in this PR and update ASCenterLayoutSpec to use this new spec behind the scenes? I really want to see this merged soon. Thanks!

[stowy]

Hey Huy, sure sorry I wasn't aware of that work. I'll get onto it in the
next couple of days.
On Tue, Feb 23, 2016 at 9:31 AM Huy Nguyen notifications@github.com wrote:

@stowy https://github.com/stowy Do you have time to fix conflicts in
this PR and update ASCenterLayoutSpec to use this new spec behind the
scenes? I really want to see this merged soon. Thanks!

—
Reply to this email directly or view it on GitHub
#1098 (comment)
.

[stowy]

@nguyenhuy I've revised the ASCenterLayoutSpec to use the ASRelativeLayoutSpec internally for the layout. Originally i made the center spec inherit from the relative spec but i thought that this exposes additional API for the center spec which may confuse things.

Let me know if you're happy with the latest approach.

[stowy]

@nguyenhuy @appleguy is there anything outstanding for this or is it good to go?

[appleguy]

{ on new line for both of these methods

[appleguy]

@stowy now that the new version of Pinterest has been branched for a little while, I am finally catching up here. Nonetheless, more conflicts had arisen, so I created a new branch to rebase and squash these commits.

The resulting commit, even with my additional changes, still comes from you as the author :). Much-deserved, given the thoughtfulness and care you put it into expanding the core framework functionality for others to enjoy!

#1405