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

Comment Detail: Add header cell #17095

Merged
merged 5 commits into from
Sep 1, 2021
Merged

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Aug 30, 2021

Refs #17087

This adds the header cell component for the new comment detail. Some notes:

  • The header cell is pretty simple, since it's a UITableViewCell with subtitle style (with properly applied text styles for both text label and detail text label). However, since this cell will be reused again later in Comment Threads, I'll create a new class for now to encapsulate all the styling and the .subtitle cell style.
  • There are two variants for the header cell content. For this PR, I'll only show the comment's post regardless if the comment is a reply or not.
    • If the current comment is a reply, I'll have to fetch the parent Comment object in order to get the both the replied author's name, and the content. This will be done in a separate PR. I've added a new entry in Comments: Create new comment details view #17087.

Preview:

Simulator Screen Shot - iPhone 8 - 2021-08-30 at 21 41 57

To test:

  • Ensure that the New Comment Detail feature flag is enabled.
  • Go to My Site > Comments, and tap on any comment. Preferably not the reply ones.
  • Verify that the header cell is displayed.
  • Tap the header cell, and verify that the post is opened correctly.

Regression Notes

  1. Potential unintended areas of impact
    n/a

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    n/a

  3. What automated tests I added (or what prevented me from doing so)
    n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@dvdchr dvdchr added this to the 18.2 milestone Aug 30, 2021
@dvdchr dvdchr requested review from aerych and ScoutHarris August 30, 2021 14:55
@dvdchr dvdchr self-assigned this Aug 30, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 30, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 30, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

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

Hey @dvdchr . Just a couple comments, but otherwise 👍 !

:shipit:

}

let readerViewController = ReaderDetailViewController.controllerWithPostID(NSNumber(value: comment.postID), siteID: siteID, isFeed: false)
navigationController?.pushFullscreenViewController(readerViewController, animated: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

As I was testing on iPad, I noticed an odd behavior which I think is related to pushFullscreenViewController. The readerViewController tries to appear full screen, then jumps down to split view. I see this is simply copied behavior from CommentViewController, and I did confirm this is an existing issue in the app store version. But maybe we can fix it with this feature? (at some point, not in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense. I'll note this down in #17087 and address this in a separate PR. Thanks! 🙂

@dvdchr dvdchr merged commit 030466b into develop Sep 1, 2021
@dvdchr dvdchr deleted the issue/17087-comment-header-cell branch September 1, 2021 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants