[ASScrollNode] Fix small bugs and add unit tests #637
Conversation
nguyenhuy
commented
Oct 24, 2017
•
nguyenhuy
commented
- Currently, ASScrollNode's calculated size is based on its parent size. However, there are cases in which the size range passed to the scroll node to calculate a layout against doesn't include the parent size. In such cases, we should clamp the parent size against the size range to make sure the range is always respected.
- When scrollable directions change, invalidate the calculated layout because these directions affect the size range that is used to calculate the content's layout.
- Add unit tests to cover these bugs and other common use cases.
Tests/ASScrollNodeTests.m
Outdated
| subnodeSizeRange = sizeRange; | ||
| subnodeSizeRange.max.width = CGFLOAT_MAX; | ||
|
|
||
| XCTAssertTrue(ASSizeRangeEqualToSizeRange(_subnode.constrainedSizeForCalculatedLayout, subnodeSizeRange)); |
There was a problem hiding this comment.
Nit: Let's use ASXCTAssertEqualSizeRanges and ASXCTAssertEqualSizes where possible, so that we get better error messages. Import ASXCTExtensions.h to get 'em.
Tests/ASScrollNodeTests.m
Outdated
| _subnode.style.preferredSize = subnodeSize; | ||
|
|
||
| ASLayout *layout = [_scrollNode layoutThatFits:sizeRange parentSize:parentSize]; | ||
| [_scrollNode layout]; |
There was a problem hiding this comment.
Another nit: I prefer to name ivars without underscores in unit test files. It makes things more readable and it's usually safe since a new instance is created for each test method.
nguyenhuy
force-pushed
the
HNTestScrollNode
branch
from
7d650c3
to
5b60aea
Compare
|
🚫 CI failed with log |
There was a problem hiding this comment.
Great to see improvements in test code. Thank you, @nguyenhuy !
Tests/ASScrollNodeTests.m
Outdated
| ASLayout *layout = [scrollNode layoutThatFits:sizeRange parentSize:parentSize]; | ||
| [scrollNode layout]; | ||
| ASLayout *layout = [self.scrollNode layoutThatFits:sizeRange parentSize:parentSize]; | ||
| [self.scrollNode layout]; |
There was a problem hiding this comment.
It seems better (perhaps) to call [scrollNode.layer layoutIfNeeded], to test the full layout stack as it is triggered in most apps?
Tests/ASScrollNodeTests.m
Outdated
|
|
||
| ASSizeRange subnodeSizeRange = sizeRange; | ||
| subnodeSizeRange.max.height = CGFLOAT_MAX; | ||
| XCTAssertEqual(scrollNode.scrollableDirections, ASScrollDirectionVerticalDirections); | ||
| ASXCTAssertEqualSizeRanges(subnode.constrainedSizeForCalculatedLayout, subnodeSizeRange); | ||
| XCTAssertEqual(self.scrollNode.scrollableDirections, ASScrollDirectionVerticalDirections); |
There was a problem hiding this comment.
Rather than self. everywhere, using the ivar like _scrollNode would be more readable (especially in property chains & methods that use it a lot).
There was a problem hiding this comment.
As a follow-up to this, I find that using non-underscored ivar names in test classes provides the best readability.
Since the object is destroyed after -tearDown is run, and since test classes usually have trivial accessors, direct ivar access is safe and convenient.
nguyenhuy
force-pushed
the
HNTestScrollNode
branch
from
d35f053
to
8d33fd4
Compare
* Add unit tests for ASScrollNode * Make sure ASScrollNode's size is clamped against its size range * Invalidate ASScrollNode's calculated layout if its scrollable directions changed * Update comment * Update CHANGELOG * Address Adlai's comments
