Skip to content

Conversation

@dvdchr
Copy link
Contributor

@dvdchr dvdchr commented May 29, 2023

Refs #20568

This implements a new (SwiftUI!) header component for Comment screens. Currently, this is only shown in Reader comments when the feature flag is enabled. Here are some previews:

Before After
header_before_light header_after_light
header_before_dark header_after_dark

⚠️ Note: The featured image thumbnail is not included yet in this PR. It will be added through a separate PR.

To test

  • Launch the app.
  • Go to App Settings > Debug and ensure that the Comment Moderation Update flag is toggled on.
  • Go to any Reader comments.
  • Verify that the new header is shown.

Regression Notes

  1. Potential unintended areas of impact
    Should be none. The feature is hidden behind a feature flag.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manually tested the changes.

  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.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 29, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr20753-df561e2
Version22.6
Bundle IDcom.jetpack.alpha
Commitdf561e2
App Center Buildjetpack-installable-builds #5118
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 29, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr20753-df561e2
Version22.6
Bundle IDorg.wordpress.alpha
Commitdf561e2
App Center BuildWPiOS - One-Offs #6094
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link

@kyleaparker kyleaparker left a comment

Choose a reason for hiding this comment

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

Looks really good to me. The only change I think we could try is a lighter material :)

@mokagio
Copy link
Contributor

mokagio commented Jun 11, 2023

Bumping this to 22.7 because I'm starting the 22.6 code freeze today and this is still a draft.

@mokagio mokagio modified the milestones: 22.6, 22.7 Jun 11, 2023
@dvdchr dvdchr marked this pull request as ready for review June 15, 2023 09:09
@dvdchr dvdchr requested a review from wargcm June 15, 2023 09:09
Copy link
Contributor

@wargcm wargcm left a comment

Choose a reason for hiding this comment

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

I checked:
✅ Light/dark mode
✅ Different accessibility sizes
✅ VoiceOver
✅ RtL language

LGTM! :shipit:

@State var showsDisclosureIndicator = true

var body: some View {
if #available(iOS 15.0, *) {
Copy link
Contributor

@kean kean Jun 22, 2023

Choose a reason for hiding this comment

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

You are hopefully going to be able to remove this (and VisualEffectView) soon. There is a discussion when to merge the deployment target bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up, and looking forward to it! I'll create a follow-up PR to remove them once the version bump PR gets merged.

@dvdchr dvdchr enabled auto-merge June 23, 2023 09:03
@dvdchr dvdchr disabled auto-merge June 23, 2023 09:03
@dvdchr dvdchr enabled auto-merge June 23, 2023 09:16
@dvdchr dvdchr merged commit d33be46 into trunk Jun 23, 2023
@dvdchr dvdchr deleted the task/20568-comment-sticky-header branch June 23, 2023 09:52
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.

7 participants