fix SIMULATE_WEB_RESPONSE not imported #449 #450

wsdwsd0829 · 2017-07-17T00:02:23Z

I could not found the correct format for "license as suggested by our bot", what should I use exactly?

CLAassistant · 2017-07-17T00:02:29Z

All committers have signed the CLA.

nguyenhuy

Thanks for contributing, @wsdwsd0829! Please change the file license as suggested by our bot and other small stuffs.

nguyenhuy · 2017-07-17T10:54:37Z

examples/ASCollectionView/Sample/ViewController.m

-      [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");


Please update the comments here and a couple of lines below to use "collectionNode"

with SIMULATE_WEB_RESPONSE, compiler will give error saying collectionView undefined, this change will follow the property to use collectionNode

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 :)

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.

nguyenhuy · 2017-07-17T10:55:23Z

examples/ASCollectionView/Sample/ViewController.m

@@ -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(), ^{


While I'm not opposing to the change at this line, I'm interested in your reasons.

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

To make minimal change, let's revert the change here (i.e change it back to 4 seconds).

nguyenhuy · 2017-08-02T14:34:31Z

examples/ASCollectionView/Sample/ViewController.m

@@ -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(), ^{


To make minimal change, let's revert the change here (i.e change it back to 4 seconds).

nguyenhuy · 2017-08-02T14:37:10Z

examples/ASCollectionView/Sample/ViewController.m

-      [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");


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 :)

nguyenhuy

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

fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449

929d118

nguyenhuy requested changes Jul 17, 2017

View reviewed changes

nguyenhuy requested changes Aug 2, 2017

View reviewed changes

wsdwsd0829 and others added 3 commits August 13, 2017 15:51

changes per review

5bbdf50

merge master

0ae8c3d

Update license of ViewController.m

2c17a73

nguyenhuy mentioned this pull request Aug 14, 2017

ASCollectionLayout to return a zero content size if its state is unavailable #509

Merged

nguyenhuy approved these changes Aug 14, 2017

View reviewed changes

nguyenhuy merged commit a929950 into TextureGroup:master Aug 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix SIMULATE_WEB_RESPONSE not imported #449 #450

fix SIMULATE_WEB_RESPONSE not imported #449 #450

wsdwsd0829 commented Jul 17, 2017 •

edited

Loading

CLAassistant commented Jul 17, 2017 •

edited

Loading

nguyenhuy left a comment

nguyenhuy Jul 17, 2017

wsdwsd0829 Jul 23, 2017

nguyenhuy Aug 2, 2017

wsdwsd0829 Aug 13, 2017

nguyenhuy Jul 17, 2017

wsdwsd0829 Jul 23, 2017 •

edited

Loading

nguyenhuy Aug 2, 2017

nguyenhuy Aug 2, 2017

nguyenhuy Aug 2, 2017

nguyenhuy left a comment

fix SIMULATE_WEB_RESPONSE not imported #449 #450

fix SIMULATE_WEB_RESPONSE not imported #449 #450

Conversation

wsdwsd0829 commented Jul 17, 2017 • edited Loading

CLAassistant commented Jul 17, 2017 • edited Loading

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

wsdwsd0829 Jul 23, 2017 • edited Loading

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

wsdwsd0829 commented Jul 17, 2017 •

edited

Loading

CLAassistant commented Jul 17, 2017 •

edited

Loading

wsdwsd0829 Jul 23, 2017 •

edited

Loading