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 regular text cells #17105

Merged
merged 3 commits into from
Sep 2, 2021
Merged

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Sep 1, 2021

Refs #17087

Adds the regular text cells for comment detail view. Some notes:

  • The cells are again, using plain UITableViewCell since the design matches the .subtitle cell style from iOS.
  • Configured table footer view to show a 1pt high plain UIView, to hide the separator for last cell.
  • Configured the table's leading inset to 20pt as per the design.

Here's a peek:
Simulator Screen Shot - iPhone 8 - 2021-09-01 at 16 04 34


Question for @mattmiklic:

  • How do we want to treat empty values on the cells? e.g. Web address on the screenshot 👆 . Should we put a placeholder text indicating that the value is empty, or should we just hide cells with empty values altogether?
  • Specific for Web address field, if we want to use a placeholder text for empty values, should we hide the external icon on the right? Since empty values shouldn't lead the user anywhere when the cell is tapped.

To test

  • Ensure the New comment detail feature flag is switched on.
  • Go to My Site > Comments, and tap on any comment.
  • Verify that the new fields: web address, email address, and IP address is there.
  • Verify that the web address can be tapped, and it shows the author's site in modal style.
  • Verify that email and IP address doesn't do anything when tapped.
  • Pick a comment where the author's URL is empty, and verify that tapping on the web address field does nothing.

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.

@dvdchr dvdchr added this to the 18.2 milestone Sep 1, 2021
@dvdchr dvdchr self-assigned this Sep 1, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 1, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 1, 2021

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

@ScoutHarris
Copy link
Contributor

This is unrelated to this PR, but I noticed an issue on an iPhone with split view (ex iPhone 12 Pro Max). When the device is rotated to landscape and back to portrait, the label is lost on the nav bar Back button.

Initial view in portrait:
port_init

Rotated to landscape:
landscape

Rotated back to portrait:
port_after

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'm not quite sure this was ready for review since there were no testing instructions and outstanding questions to Matt. But as it is, LGTM .

@dvdchr
Copy link
Contributor Author

dvdchr commented Sep 2, 2021

there were no testing instructions

You're right 🤦 . Although it's practically a visual check, I forgot to add testing instructions for this. I've now added it for documentation purposes. Thank you!

@dvdchr
Copy link
Contributor Author

dvdchr commented Sep 2, 2021

This is unrelated to this PR, but I noticed an issue on an iPhone with split view (ex iPhone 12 Pro Max). When the device is rotated to landscape and back to portrait, the label is lost on the nav bar Back button.

Noted. I'll address this in a separate PR! For now, I've tracked this in #17087. 👍

@dvdchr
Copy link
Contributor Author

dvdchr commented Sep 2, 2021

I'll go ahead and merge this for now. Will address follow-up suggestions from @mattmiklic through a separate PR! 🙂

@dvdchr dvdchr merged commit 17076b2 into develop Sep 2, 2021
@dvdchr dvdchr deleted the issue/17087-regular-text-cells branch September 2, 2021 12:53
@mattmiklic
Copy link
Member

Hey @dvdchr --

How do we want to treat empty values on the cells? e.g. Web address on the screenshot 👆 . Should we put a placeholder text indicating that the value is empty, or should we just hide cells with empty values altogether?

Good catch. I'm leaning toward saying we should hide the cell if there's no content. We could do a placeholder like "None" or "Not provided" but it seems a little bit superfluous. AFAIK web address is the only cell where this applies since all the others are required.

Specific for Web address field, if we want to use a placeholder text for empty values, should we hide the external icon on the right? Since empty values shouldn't lead the user anywhere when the cell is tapped.

Since I think we should hide the field this is moot, but in case we do decide to use a placeholder, yes we should hide the external icon since there's nothing to link to.

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.

3 participants