Skip to content

Conversation

@hassaanelgarem
Copy link
Contributor

Part of #19453

Description

This PR implements the overlay for new users. The design for this overlay is different from the existing overlays, which required adding a way to customize the generic overlay.

⚠️ Disclaimer

The focus of this PR is only on the design of the overlay.
The menu card and showing the overlay on login will be implemented in a future PR.

Screenshots

Light Mode Dark Mode
light dark

Pre-testing Instructions

  • To disable the learn more button:
    • Go to JetpackFullscreenOverlayGeneralViewModel.swift line 183
    • Replace this line with return nil
    • This won't be needed in production since the learn more button will be disabled remotely by then
  • To always show the overlay even if it was shown before:
    • Go to JetpackFeaturesRemovalCoordinator.swift line 132
    • Change the default value to true

Testing Instructions

  1. Open the app
  2. Open the debug menu and enable "Jetpack Features Removal Phase for New Users"
  3. Navigate to My Site
  4. Close and re-open the app
  5. The overlay should be displayed
  6. Make sure that the overlay matches the design

Regression Notes

  1. Potential unintended areas of impact
    The design of other overlays.

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

  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.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 10, 2023

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19881-483007d on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 10, 2023

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19881-483007d on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Works as described! A couple of UI things I noticed:

  1. Are the buttons supposed to be leading aligned or center aligned in compact height mode? Currently they're aligned to the leading margin, but just wanted to make sure.

Simulator Screen Shot - iPhone 14 Pro - 2023-01-10 at 11 28 00

  1. If you adjust the text size to use larger accessibility sizes, the labels in the secondary view overlap each other.

static let notificationsContinueButtonTitle = NSLocalizedString("jetpack.fullscreen.overlay.notifications.continue.title",
value: "Continue to Notifications",
comment: "Title of a button that dismisses an overlay and displays the Notifications screen.")
static let continueButtonTitle = NSLocalizedString("jetpack.fullscreen.overlay.phaseThree.general.continue.title",
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]

Suggested change
static let continueButtonTitle = NSLocalizedString("jetpack.fullscreen.overlay.phaseThree.general.continue.title",
static let continueButtonTitle = NSLocalizedString("jetpack.fullscreen.overlay.general.continue.title",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally did not update this to not force a new translation of an already translated string. But now I'm having second thoughts. Not sure what's worse, having an inaccuracy in the key or translating an already translated string. What do you think?
Related thread: p1650995535103199/1650995010.603439-slack-CC7L49W13

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the thread link, that makes a lot of sense.

Hmmm. In this case the renaming is a super minor change (i.e. it doesn't noticeably improve the readability etc), so maybe it's best to leave it as is.

@hassaanelgarem
Copy link
Contributor Author

Are the buttons supposed to be leading aligned or center aligned in compact height mode? Currently they're aligned to the leading margin, but just wanted to make sure.

@momo-ozawa That's the expected behavior, yes. Below is a screenshot from Figma:

iOS Overlay

If you adjust the text size to use larger accessibility sizes, the labels in the secondary view overlap each other.

Nice catch! This is fixed in 155a642 & 483007d 👍

@momo-ozawa
Copy link
Contributor

Nice catch! This is fixed in 155a642 & 483007d 👍

Re-tested, and can confirm that overlapping issue is resolved! Thanks 🙇‍♀️

@hassaanelgarem hassaanelgarem merged commit 35ff421 into trunk Jan 11, 2023
@hassaanelgarem hassaanelgarem deleted the task/19453-new-users-phase branch January 11, 2023 21:50
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.

4 participants