Skip to content

Onboarding: Send set*() messages to native apps when step is skipped #1648

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

Conversation

noisysocks
Copy link
Contributor

@noisysocks noisysocks commented Apr 24, 2025

Asana Task/Github Issue: https://app.asana.com/1/137249556945/project/1209121419454298/task/1209906221282239

Description

Modifies the Onboarding special page so that the setBookmarksBar, setSessionRestore, setShowHomeButton, and setAdBlocking messages are sent with params.enabled = false when the user clicks Skip during onboarding. This allows the native apps to fire a pixel when the user skips one of these, if desired.

The requestDockOptIn, requestImport, and requestSetAsDefault messages remain as-is. These aren’t reversible actions so it doesn’t make sense to fire a message with params.enabled = false.

Testing Steps

  1. Navigate to https://deploy-preview-1648--content-scope-scripts.netlify.app/build/pages/onboarding/?adBlocking=enabled.
  2. Open DevTools.
  3. Work your way through onboarding, skipping everything.
  4. Notice that you see unhandled notification warnings in the console with params.enabled = false when skipping:
    • “Browse faster with even fewer ads”
    • “Show a bookmarks bar with your favorite sites”
    • “Restore previous websites on startup”
    • “Add a shortcut to your homepage in the toolbar"

Checklist

Please tick all that apply:

  • I have tested this change locally
  • I have tested this change locally in all supported browsers
  • This change will be visible to users
  • I have added automated tests that cover this change
  • I have ensured the change is gated by config
  • This change was covered by a ship review
  • This change was covered by a tech design
  • Any dependent config has been merged

@noisysocks noisysocks added the enhancement New feature or request label Apr 24, 2025
@noisysocks noisysocks requested review from shakyShane, mgurgel and a team as code owners April 24, 2025 04:30
Copy link

netlify bot commented Apr 24, 2025

Deploy Preview for content-scope-scripts ready!

Name Link
🔨 Latest commit 7c1fd07
🔍 Latest deploy log https://app.netlify.com/sites/content-scope-scripts/deploys/68182bfaad4d7d0008ed4e1d
😎 Deploy Preview https://deploy-preview-1648--content-scope-scripts.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Apr 24, 2025

Temporary Branch Update

The temporary branch has been updated with the latest changes. Below are the details:

Please use the above install command to update to the latest version.

Copy link

github-actions bot commented Apr 24, 2025

[Beta] Generated file diff

Time updated: Mon, 05 May 2025 03:10:30 GMT

Integration
    - integration/pages/onboarding/dist/index.js

File has changed

Windows
    - windows/pages/onboarding/dist/index.js

File has changed

Apple
    - dist/pages/onboarding/dist/index.js

File has changed

Copy link
Contributor

@mgurgel mgurgel left a comment

Choose a reason for hiding this comment

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

Looks good and works locally 👍

@mgurgel
Copy link
Contributor

mgurgel commented Apr 29, 2025

@shakyShane When we chatted about this approach on MM, were there any concerns other than enabled=false potentially causing regressions?

@noisysocks noisysocks force-pushed the randerson/send-messages-when-some-system-settings-are-skipped branch from ac5e346 to 7c1fd07 Compare May 5, 2025 03:09
@noisysocks noisysocks merged commit 23e7acf into main May 5, 2025
14 checks passed
@noisysocks noisysocks deleted the randerson/send-messages-when-some-system-settings-are-skipped branch May 5, 2025 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants