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

Add NTP Customization Settings onboarding popover #3428

Open
wants to merge 3 commits into
base: dominik/ntp-search-bar
Choose a base branch
from

Conversation

ayoy
Copy link
Collaborator

@ayoy ayoy commented Oct 21, 2024

Task/Issue URL: https://app.asana.com/0/0/1208583505730578/f

Description:
This change adds a popover that is displayed once for every user over the 'Customize' button on the NTP.
It's not a part of the experiment nor it's related to the search bar project. We're adding the onboarding to
increase engagement with NTP customization settings.

Steps to test this PR:

  1. Update DefaultNewTabPageSearchBoxExperimentCohortDecider.cohort getter body to just return .experiment
  2. Launch the app.
  3. Verify that a popover is displayed over the Customize button.
  4. Click the popover, verify that the popover is gone and settings are opened.
  5. Click Main Menu -> Debug -> Reset Data -> Reset Home Page Settings Onboarding and open a new NTP
  6. Click the popover, verify that the popover is gone and settings are opened.
  7. Repeat step 4.
  8. When popover is visible, dismiss it and verify that the popover is gone and nothing else happens (settings are not open).
  9. Repeat step 4.
  10. When popover is visible, click 'Customize' button and verify that the popover is gone and settings are opened.
  11. Repeat step 4.
  12. When popover is visible, search for something and verify that the popover is gone.
  13. Relaunch the app and verify that the popover doesn't show up.

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

Comment on lines +66 to +70
if let title = viewModel.title {
messageWithTitleBody(title)
} else {
messageBody
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change to this file is the support for title parameter.

When title is present, we display a different message body, consisting of title and message.
When title is not present, we display the old view with just the message.


@State var isHovering: Bool = false
@Binding var isSettingsVisible: Bool
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been removed in favor of using settingsVisibilityModel.

@ayoy ayoy force-pushed the dominik/ntp-search-bar-settings-cta branch from 0ef80c9 to 56e51ee Compare October 21, 2024 20:09
Copy link

github-actions bot commented Oct 21, 2024

Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against 7b496da

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants