-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix SIMULATE_WEB_RESPONSE not imported #449 #450
fix SIMULATE_WEB_RESPONSE not imported #449 #450
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing, @wsdwsd0829! Please change the file license as suggested by our bot and other small stuffs.
[strongSelf->_collectionView performBatchUpdates:^{ | ||
[strongSelf->_collectionView insertSections:[[NSIndexSet alloc] initWithIndexesInRange:NSMakeRange(0, 100)]]; | ||
[strongSelf->_collectionNode performBatchUpdates:^{ | ||
[strongSelf->_collectionNode insertSections:[[NSIndexSet alloc] initWithIndexesInRange:NSMakeRange(0, 100)]]; | ||
} completion:nil]; | ||
NSLog(@"ViewController finished updating collectionView"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the comments here and a couple of lines below to use "collectionNode"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with SIMULATE_WEB_RESPONSE, compiler will give error saying collectionView undefined, this change will follow the property to use collectionNode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand the change here. I was suggesting to update the logs to use "collectionNode":
NSLog(@"ViewController finished updating collectionNode")
and NSLog(@"ViewController is nil - won't update collectionNode")
. Sorry for the confusion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if we keep using 2s it will cause crash at L115 ASCollectionLayout, because poping VC and layout happen at same time; So I keep 4.
- (CGSize)collectionViewContentSize
{
...
ASDisplayNodeAssertNotNil(_layout, @"Collection layout state should not be nil at this point")
...
Let me know if it's potential bug and how to fix it. Thanks.
@@ -81,7 +81,7 @@ - (void)viewDidLoad | |||
}; | |||
|
|||
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC)), dispatch_get_main_queue(), mockWebService); | |||
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ | |||
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(4 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'm not opposing to the change at this line, I'm interested in your reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know why need last dispatch, current behavior is when click push, the collection node/view will display after 2 second, then pop after another 2s (thus 4 total)
Actually I think we do not need to pop automatically, this change just try to make minimal change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make minimal change, let's revert the change here (i.e change it back to 4 seconds).
@@ -81,7 +81,7 @@ - (void)viewDidLoad | |||
}; | |||
|
|||
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC)), dispatch_get_main_queue(), mockWebService); | |||
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ | |||
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(4 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make minimal change, let's revert the change here (i.e change it back to 4 seconds).
[strongSelf->_collectionView performBatchUpdates:^{ | ||
[strongSelf->_collectionView insertSections:[[NSIndexSet alloc] initWithIndexesInRange:NSMakeRange(0, 100)]]; | ||
[strongSelf->_collectionNode performBatchUpdates:^{ | ||
[strongSelf->_collectionNode insertSections:[[NSIndexSet alloc] initWithIndexesInRange:NSMakeRange(0, 100)]]; | ||
} completion:nil]; | ||
NSLog(@"ViewController finished updating collectionView"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand the change here. I was suggesting to update the logs to use "collectionNode":
NSLog(@"ViewController finished updating collectionNode")
and NSLog(@"ViewController is nil - won't update collectionNode")
. Sorry for the confusion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now. Thank you for the contribution!
* fix SIMULATE_WEB_RESPONSE not imported (Reported in TextureGroup#449). * changes per review * Update license of ViewController.m
I could not found the correct format for "license as suggested by our bot", what should I use exactly?