Skip to content
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

[Stats Refresh] Insights Comments: add detail list view #11181

Merged
merged 5 commits into from
Mar 5, 2019

Conversation

ScoutHarris
Copy link
Contributor

@ScoutHarris ScoutHarris commented Mar 4, 2019

Fixes #11160

On the Insights Comments card, selecting View more will now show a full list of Comments.

  • Currently, there is no API call to get the full list of Comments for either tab. So for now, this displays the data fetched from the "top" Comments API call used for the Insights Comments card. (cc @jklausa issue [Stats Refresh] Insights Comments detail list: correct data with new API #11182)
  • When View more is selected, the filter tab (and thus the data) will be the tab selected on the overview card.
  • Pull to refresh updates the data.
  • If one tab has data and the other does not, the empty tab will display No data yet.

NOTE: The UI for the details view still needs to be updated. That will be done with #11165.

To test:


  • On a site with more than 6 comments, go to Insights > Comments > View more.
  • Verify:
    • The tab titles match the overview card.
    • The selected tab match the overview card.

comments_list


  • On the Posts and Pages tab, selecting a row opens a web view for that post/page.

@ScoutHarris ScoutHarris added this to the 12.0 milestone Mar 4, 2019
@ScoutHarris ScoutHarris self-assigned this Mar 4, 2019
@ScoutHarris ScoutHarris requested a review from a user March 4, 2019 19:11
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hey @ScoutHarris - this is looking pretty good to me! I wanted to share my findings.

✅ Branch builds
✅ Tests pass
✅ Tapping View More behaves as expected, and honors the selected tab as you described.
✅ Tapping a row from Posts and Pages opens a web view

I have two questions:

  1. If I tap a row, can the expected behavior open the site, or should the URL include the specific comment?
  2. If Posts and Pages was selected when I navigate to Comments via View More, I'm observing that Authors is selected when I navigate back. Should that original selection be preserved?

Please let me know if you need additional information. Thanks!

@ScoutHarris
Copy link
Contributor Author

Hey @stevebaranski ! Thanks for the review.

If I tap a row, can the expected behavior open the site, or should the URL include the specific comment?

For Posts & Pages, the row selection is the same behavior as selecting a row from the Insights Comments card, which is it opens the post URL. If @SylvesterWilmott desires different behavior, it can be changed in a future PR (with some accompanying backend data alteration).

If Posts and Pages was selected when I navigate to Comments via View More, I'm observing that Authors is selected when I navigate back. Should that original selection be preserved?

The Android implementation does not persist the filter selection from details back to Insights, so I copied that behavior. However, if @SylvesterWilmott wishes it to be persisted, it can be done in a separate PR (as it should be the same for Insights Followers).

@ghost
Copy link

ghost commented Mar 5, 2019

@ScoutHarris sounds good! I'm happy to approve once he provides that clarity.

For Posts & Pages, the row selection is the same behavior as selecting a row from the Insights Comments card, which is it opens the post URL.

Acknowledged. I think this could be a nice enhancement, provided we have that information at our disposal. If we choose to create an issue for this, I agree that it should be considered in a future release.

The Android implementation does not persist the filter selection from details back to Insights, so I copied that behavior.

I'm happy to defer to Sylvester here. Personally I think this one warrants resolution prior to shipping, but I'm open to considering it subsequent to this PR. As it stands, the behavior is inconsistent:
• A → B → A
• A' → B' → A

@SylvesterWilmott
Copy link

Looks awesome @ScoutHarris !

If I tap a row, can the expected behavior open the site, or should the URL include the specific comment?

I'm a little confused, the rows don't refer to specific comments but instead only to that post and the number of comments it has.

If Posts and Pages was selected when I navigate to Comments via View More, I'm observing that Authors is selected when I navigate back. Should that original selection be preserved?

You're right @stevebaranski, we should preserve the tab selection. We can create an issue for this one but it isn't critical.

@ghost
Copy link

ghost commented Mar 5, 2019

we should preserve the tab selection. We can create an issue for this one but it isn't critical.

I have added this to the list of outstanding issues in #10840. Proceeding to approve.

@ScoutHarris
Copy link
Contributor Author

I have added this to the list of outstanding issues

Thanks @stevebaranski !

@ScoutHarris ScoutHarris merged commit b572638 into develop Mar 5, 2019
@ScoutHarris ScoutHarris deleted the feature/11160-insights_comments_view_more branch March 5, 2019 18:20
@ScoutHarris ScoutHarris mentioned this pull request Jun 4, 2019
79 tasks
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