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

Reader Comments: Show separator for last leaf comments #17549

Merged
merged 4 commits into from
Nov 25, 2021

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Nov 24, 2021

Refs #17476

This PR implements separator logic for the comment thread. Additionally, the separator is always hidden for the last row (as per the design).

I couldn't find a better term for this 😅 , but a "last leaf" comment is the last descendant of a top-level comment. I've also added a note in the code, but here's an example:

- comment 1
  - comment 2
    - comment 3 --> this is the last leaf of comment 1.
- comment 4
  - comment 5
    - comment 6
  - comment 7 --> this is the last leaf of comment 4.
- comment 8

To Test

  • Enable the newCommentThread feature flag.
  • Go to Reader > any comment thread.
  • 🔍 Verify that the cell separators are correctly shown for every "last leaf" comment.
  • Rotate the device to landscape.
  • 🔍 Verify that the cell separators are displayed correctly.

Regression Notes

  1. Potential unintended areas of impact
    n/a. Feature is hidden behind a flag.

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

  3. What automated tests I added (or what prevented me from doing so)
    n/a. Feature is hidden behind a flag.

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.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 24, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 24, 2021

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

Comment on lines +41 to +42
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Classic Xcode 13 formatting. 🙄

@ScoutHarris
Copy link
Contributor

I noticed one visual oddity. On an iPad (in this case iPad Pro 9.7-inch iOS15):

  • In portrait, view post comments.
  • Rotate to landscape.
  • Notice the separator "artifacts" on the left.

landscape

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.

I noted one UI issue. But since this is a WIP I'll go ahead and approve to not hold you up.

@dvdchr
Copy link
Contributor Author

dvdchr commented Nov 25, 2021

I noted one UI issue.

Thank you! This should be fixed in 31b7f3f. I noticed that this happened only after rotating from portrait to landscape. It doesn't happen when opening the comment thread while already in landscape orientation 😅. Here's a screenshot after rotating from portrait to landscape:

Simulator Screen Shot - iPad Air (4th generation) - 2021-11-25 at 16 02 20

I've also tested to ensure that this does not cause any issues in iPhone. 🙂

@dvdchr dvdchr enabled auto-merge November 25, 2021 09:06
@dvdchr dvdchr disabled auto-merge November 25, 2021 09:16
@dvdchr dvdchr enabled auto-merge November 25, 2021 09:40
@dvdchr dvdchr merged commit b03f8f0 into develop Nov 25, 2021
@dvdchr dvdchr deleted the issue/17475-table-separators branch November 25, 2021 11:13
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