Skip to content

Deprecating WPTableViewController (YAY!) - #3865

Merged
jleandroperez merged 23 commits into
developfrom
issue/nukes-table-view-controller
Jun 15, 2015
Merged

Deprecating WPTableViewController (YAY!)#3865
jleandroperez merged 23 commits into
developfrom
issue/nukes-table-view-controller

Conversation

@jleandroperez

Copy link
Copy Markdown
Contributor

Details:

In this PR we'll nuke WPTableViewController, and update the last ViewController that was still depending on it, to rely on WPTableViewHandler + WPContentSyncHelper instead.

This is based on the work already done by Eric in ReaderCommentsViewController.

Test Scenario 1: Infinite Scroll

  1. Launch WPiOS
  2. Tap over My Sites
  3. Pick any of your blogs
  4. Tap over Comments
  5. Scroll all the way down

Expected: a spinner shows up at the bottom, and the app begins loading more comments

Test Scenario 2: Empty State

  1. Launch WPiOS
  2. Tap over My Sites
  3. Pick any of your blogs that does not have any comments
  4. Tap over Comments

Expected: A label shows up onscreen, indicating that there are no comments to be displayed.

Test Scenario 3: Pull to Refresh

  1. Launch WPiOS
  2. Tap over My Sites
  3. Pick any of your blogs
  4. Tap over Comments
  5. On the web, add a new comment
  6. Perform the Pull to Refresh gesture

Expected: A spinner shows up onscreen, and after a couple seconds, the new comment shows up onscreen.

@aerych can i poke you with this one?. I think you'd love the WPTableViewController part!

Thank you!

@jleandroperez jleandroperez added this to the 5.4 milestone Jun 15, 2015

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.

Maybe move the import into the implementation and define it here via a @class ?

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.

Might be nice to have consts for the batch size and the spam status in the predicate. Maybe something for another PR targeting cleanup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Turns out, there was already a spam constant in Comment.h. Updating right here, thanks!

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.

Thinking about this a bit... In the reader, it makes sense to fetch comments fresh everytime its comment list loads because they are post centric and we always expect different results. But I think in a blog's Comments list, we expect the content to change less frequently. The previous implementation included a mechanism for automatically refreshing only "if needed," which basically meant at least 5 minutes after the last time the list was synced. There is a similar implementation in the current version of the Post and Page lists. Would it make sense to retain something along the same lines here?

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.

This guy existed so WPTableViewController subclasses could specify what sort of content needed to be fetched. If we wanted to, we could kill it to simplify things a bit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's being used, currently, by the fetchRequest getter. I've just kept it because, in Notifications, we also have it there. Felt consistent! =D

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.

👍

@jleandroperez

Copy link
Copy Markdown
Contributor Author

@aerych "Refresh every 5 minutes"'s mechanism has been brought back. However, i've found out that Blog's lastCommentSync's property is not being updated anywhere.

I've filed #3867 to address that one, thank you!

@aerych

aerych commented Jun 15, 2015

Copy link
Copy Markdown
Contributor

Hiya @jleandroperez :) I tested the three scenarios and they worked great. Thanks for the fast code changes, I could barely stay ahead of you in the review :D.
:shipit:

@jleandroperez

Copy link
Copy Markdown
Contributor Author

@aerych thanks so much!!

So long, WPTableViewController!!

jleandroperez added a commit that referenced this pull request Jun 15, 2015
…-controller

Deprecating WPTableViewController (YAY!)
@jleandroperez
jleandroperez merged commit 0df708a into develop Jun 15, 2015
@jleandroperez
jleandroperez deleted the issue/nukes-table-view-controller branch June 15, 2015 20:41
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.

2 participants