Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Implements latest post summary in Insights #315

Merged
merged 29 commits into from
Sep 22, 2015

Conversation

astralbodies
Copy link
Contributor

Closes #289
New UI:
2015-09-16_14-03-15

2015-09-16_14-02-06

Test scenarios:

  1. No data.
  2. Slow/crappy data (using link conditioner).
  3. iPad and iPhone - ensure rotations work right.
  4. Sites with no posts.
  5. iOS 9, 8 and 7 (I'll take care of most of this).

How to test:

  • Local deployment if you're into Xcode 7.
  • StatsDemo which I'll be sending out soon.

Needs Review: @jancavan, @jleandroperez, @daniloercoli

@astralbodies
Copy link
Contributor Author

Updated the Travis build with this fix to let xctool actually run the unit tests properly - facebookarchive/xctool#528 (comment)

@astralbodies astralbodies removed the UI label Sep 17, 2015
@astralbodies
Copy link
Contributor Author

StatsDemo build 39 has this PR included.

switch (indexPath.row) {
case 1: // Views
{
cell.categoryIconLabel.text = @"";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is me not mentioning how fragile the looks like!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, I KNOW 😄

@jleandroperez
Copy link
Contributor

Not sure exactly why, but i'm seeing this in the Console logs (Xcode 7):

Sep 21 16:51:25  StatsDemo[25671] <Error>: CGContextSaveGState: invalid context 0x0. If you want to see the backtrace, please set CG_CONTEXT_SHOW_BACKTRACE environmental variable.
Sep 21 16:51:25  StatsDemo[25671] <Error>: CGContextTranslateCTM: invalid context 0x0. If you want to see the backtrace, please set CG_CONTEXT_SHOW_BACKTRACE environmental variable.
Sep 21 16:51:25  StatsDemo[25671] <Error>: CGContextRestoreGState: invalid context 0x0. If you want to see the backtrace, please set CG_CONTEXT_SHOW_BACKTRACE environmental variable.

@jleandroperez
Copy link
Contributor

@astralbodies I've got a post with 0 likes and 0 comments (plus a bunch views). Tapping over any of the rows always leads to the same details view, would that be expected?

@jleandroperez
Copy link
Contributor

And yet another comment (sorry about the spam!). The Post Summary's Details view shows the Post Title within the View Controller's title field.

It's way too easy to get that string clipped. Comment / suggestion, would it be possible to perhaps reuse the Latest post summary cell (or a slice of it), so that we get at least two lines to render the title (+ a smaller font) ?.

This is how my last post looks like:

simulator screen shot sep 21 2015 4 57 10 pm

@astralbodies
Copy link
Contributor Author

Re: tapping the rows in the insights getting to the same post details - that is correct behavior for right now. I did open another issue for the post details title in the navigation bar.

Thanks for reviewing @jleandroperez. I need to make the view controllers smaller and it is on my radar soon. :stabby:

:shipit: ?

@jleandroperez
Copy link
Contributor

Thanks for checking @astralbodies!

:shipit:

astralbodies added a commit that referenced this pull request Sep 22, 2015
…st-post-summary

Implements latest post summary in Insights
@astralbodies astralbodies merged commit 6318029 into develop Sep 22, 2015
@astralbodies astralbodies deleted the issue/289-insights-latest-post-summary branch September 22, 2015 16:35
@jancavan
Copy link

jancavan commented Oct 4, 2015

@astralbodies great job adding this module to Insights! 2 minor comments :)

  1. post title text should be a link that jumps to the blog post itself
  2. Likes and Comments should not be links since we only show data for Views on the post summary pages

@astralbodies
Copy link
Contributor Author

Thanks @jancavan - I'll make this another issue 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants