-
Notifications
You must be signed in to change notification settings - Fork 9
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
bookmarks onboarding #1420
bookmarks onboarding #1420
Conversation
# Conflicts: # DuckDuckGo/Common/Utilities/UserDefaultsWrapper.swift
@SabrinaTardio I added a test section for closing the prompt. Duplicates some of the earlier testing steps a little, but is more explicit about what happens if you click outwith the prompt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I left a couple of comments. The main thing I think you forgot to re add the code to randomise the cohort!
DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Outdated
Show resolved
Hide resolved
...ompt/BookmarksBarPromptAssets.xcassets/BookmarksBarIllustration.imageset/Bookmarks-Bar 1.svg
Outdated
Show resolved
Hide resolved
|
||
extension BookmarksBarPromptViewController: BookmarksBarPromptDelegate { | ||
|
||
func rejectBookmarksBar() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason the view controller need know this actions? Can’t be handled directly by the model so that it can take complete ownership of the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the dismiss()
function is a controller extension. I guess the model could just know about the view controller, but that seems nasty. This feels like controller work rather than view model work to me really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case I would have just the dismiss action as a separate func on the view controller and the actual action on the view model. And the view model can call dismiss on the view controller through the delegate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There you go, is that what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Task/Issue URL: https://app.asana.com/0/72649045549333/1204685656797210/f
Tech Design URL:
CC:
Description:
Adds bookmarks bar onboarding.
Useful commands:
defaults read com.duckduckgo.macos.browser.debug
and look for defaults starting withpixel.experiment
defaults write com.duckduckgo.macos.browser.debug bookmarks.bar.prompt.shown -bool false
to reset the promptdefaults write com.duckduckgo.macos.browser.debug pixel.experiment.cohort "variant1"
to set the cohort to the testing variantdefaults write com.duckduckgo.macos.browser.debug pixel.experiment.enrollment.date -date "2023-07-23 13:46:46 +0000"
to set the enrolment dateSteps to test this PR:
Triggering the prompt
./clean-app.sh debug
) run the app. Get through onboarding. Check the defaults and you should seepixel.experiment.installed
set to1
pixel.experiment.cohort
and it should be set tocontrol
.Closing the prompt
Pixels
m_mac_bookmarksbarexperiment_enrollment
m_mac_bookmarksbarexperiment_firstinteraction
m_mac_bookmarksbarexperiment_interaction2to8days
m_mac_bookmarksbarexperiment_searched4to8days
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation