Skip to content

[Stats Refresh] Insights Comments > Posts & Pages: show in web view - #10743

Merged
ScoutHarris merged 6 commits into
developfrom
feature/10687-insights_comments_webview
Jan 7, 2019
Merged

[Stats Refresh] Insights Comments > Posts & Pages: show in web view #10743
ScoutHarris merged 6 commits into
developfrom
feature/10687-insights_comments_webview

Conversation

@ScoutHarris

@ScoutHarris ScoutHarris commented Jan 3, 2019

Copy link
Copy Markdown
Contributor

Ref #10687

To test:

  • Go to Stats > Insights > Comments.
  • Select Posts and Pages.
  • Tap on a row.
  • Verify the post/page is displayed in a web view.

webview

@ScoutHarris ScoutHarris added this to the 11.6 milestone Jan 3, 2019
@ScoutHarris ScoutHarris self-assigned this Jan 3, 2019
frosty
frosty approved these changes Jan 4, 2019
View reviewed changes

@frosty frosty 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.

Works as expected! 👍

I noticed that there's a retain cycle with StatsTotalRows, but I'm not quite sure where it's coming from. If you navigate in and out of the stats section a couple of times, you get a load of the rows hanging about:

screenshot 2019-01-04 at 12 14 59

I thought perhaps it was because the StatsTotalRowDelegate and SiteStatsInsightsDelegate properties need to be weak, but changing them both doesn't fix it.

Anyway, I don't think that should block this PR, as I don't think it introduced the issue – just something to be aware of!

Comment thread WordPress/Classes/ViewRelated/Stats/Shared Views/StatsTotalRow.swift Outdated
private var dataBarMaxWidth: Float = 0.0
private typealias Style = WPStyleGuide.Stats

var delegate: StatsTotalRowDelegate?

@frosty frosty Jan 4, 2019

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.

Should we make this weak?

@ScoutHarris ScoutHarris Jan 4, 2019

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.

Wow. It seems I failed on all the delegates. 🤦‍♂️ I've fixed this one. I'll go back and review the others and look at the StatsTotalRows retention. Thank you so much for noticing that!!

@SylvesterWilmott

SylvesterWilmott commented Jan 4, 2019

Copy link
Copy Markdown

Looks good!

@ScoutHarris
ScoutHarris changed the base branch from feature/10708-stat_row_data_bar to develop January 7, 2019 22:01
@wpmobilebot

wpmobilebot commented Jan 7, 2019

Copy link
Copy Markdown
Contributor
1 Warning
⚠️ Podfile: reference to a commit hash

Generated by 🚫 Danger

@ScoutHarris
ScoutHarris merged commit 2106b28 into develop Jan 7, 2019
@ScoutHarris
ScoutHarris deleted the feature/10687-insights_comments_webview branch January 7, 2019 22:31
@ScoutHarris ScoutHarris mentioned this pull request Jan 21, 2019
Closed
45 tasks
@ScoutHarris ScoutHarris mentioned this pull request Jun 4, 2019
Closed
79 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jklausa jklausa Awaiting requested review from jklausa
@SylvesterWilmott SylvesterWilmott Awaiting requested review from SylvesterWilmott
1 more reviewer
@frosty frosty frosty approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ScoutHarris ScoutHarris

Labels

None yet

Projects

None yet

Milestone

11.6

Development

Successfully merging this pull request may close these issues.

4 participants

@ScoutHarris @SylvesterWilmott @wpmobilebot @frosty