Skip to content

[ASScrollNode] Fix small bugs and add unit tests#637

Merged
nguyenhuy merged 11 commits into
TextureGroup:masterfrom
nguyenhuy:HNTestScrollNode
Dec 1, 2017
Merged

[ASScrollNode] Fix small bugs and add unit tests#637
nguyenhuy merged 11 commits into
TextureGroup:masterfrom
nguyenhuy:HNTestScrollNode

Conversation

@nguyenhuy

@nguyenhuy nguyenhuy commented Oct 24, 2017

Copy link
Copy Markdown
Member
  • 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.

@nguyenhuy
nguyenhuy requested a review from appleguy October 24, 2017 18:30

@Adlai-Holler Adlai-Holler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful! Comments and unit tests and useful fixes 💯

Comment thread Tests/ASScrollNodeTests.m Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Let's use ASXCTAssertEqualSizeRanges and ASXCTAssertEqualSizes where possible, so that we get better error messages. Import ASXCTExtensions.h to get 'em.

Comment thread Tests/ASScrollNodeTests.m Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maicki maicki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks <3

@ghost

ghost commented Oct 26, 2017

Copy link
Copy Markdown

🚫 CI failed with log

@appleguy appleguy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see improvements in test code. Thank you, @nguyenhuy !

Comment thread Tests/ASScrollNodeTests.m Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems better (perhaps) to call [scrollNode.layer layoutIfNeeded], to test the full layout stack as it is triggered in most apps?

Comment thread Tests/ASScrollNodeTests.m Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than self. everywhere, using the ivar like _scrollNode would be more readable (especially in property chains & methods that use it a lot).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
nguyenhuy merged commit bccde6c into TextureGroup:master Dec 1, 2017
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants