-
Notifications
You must be signed in to change notification settings - Fork 236
Free scan CTA experiment (MNTOR-3341) #4787
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
Preview URL 🚀 : https://blurts-server-pr-4787-mgjlpikfea-uk.a.run.app |
const variableValue = | ||
nimbusConfig.features[featureId].variables[variableName].default; | ||
return ` "${variableName}": ${typeof variableValue === "string" ? `"${variableValue}"` : variableValue},\n`; |
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.
This change was needed, because the objects of defaultExperimentData
in the generated experiments.ts
file had an invalid syntax for string values.
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.
Oh nice catch!
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.
That's quite a few potential variants - thanks for the tests, they give me more confidence :)
enabled: | ||
description: If the feature is enabled | ||
type: Boolean | ||
default: false |
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.
@rhelmer Is this still necessary? I think the ctaWithEmail
variant is what we have today anyway, so the enabled
flag might be redundant?
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.
I thought it was helpful to be able to check if the experiment is enabled
. Like for example only conditionally appending UTM parameters if the experiment is running.
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.
Hmm yeah, also just realised that Rob's work on QA enabling experiments depends on this. Which also makes me wonder - how would that deal with this situation? How would QA be able to test specific variants? I'll wake up the relevant Slack thread I suppose.
config/nimbus.yaml
Outdated
ctaOnly: Only show a CTA button with the default label | ||
ctaOnlyAlternativeLabel: Only show a CTA button with an alternative label |
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.
I think there's no need to be indirect in the Nimbus UI, so this might make experiment evaluation easier?
ctaOnly: Only show a CTA button with the default label | |
ctaOnlyAlternativeLabel: Only show a CTA button with an alternative label | |
ctaOnly: Only show a CTA button with the label "Get free scan" | |
ctaOnlyAlternativeLabel: Only show a CTA button with the label "Sign in to get free scan" |
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.
I agree — updated in 6a9d2dd.
const variableValue = | ||
nimbusConfig.features[featureId].variables[variableName].default; | ||
return ` "${variableName}": ${typeof variableValue === "string" ? `"${variableValue}"` : variableValue},\n`; |
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.
Oh nice catch!
const inputField = screen.getAllByTestId("signup-form-input"); | ||
expect(inputField[0]).toBeInTheDocument(); |
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.
Since test ID is a last resort, wouldn't we be able to select this through the label text? (Thereby ensuring that screen reader users still get that announced to them?)
const inputField = screen.getAllByTestId("signup-form-input"); | |
expect(inputField[0]).toBeInTheDocument(); | |
const inputField = screen.getAllByLabelText("Enter your email address to check for data breach exposures and sites selling your info."); | |
expect(inputField[0]).toBeInTheDocument(); |
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.
Updated in deddd51.
Cleanup completed - database 'blurts-server-pr-4787' destroyed, cloud run service 'blurts-server-pr-4787' destroyed |
References:
Jira: MNTOR-3341
Figma: https://www.figma.com/design/YqpVpUbSTzZMj9Jbp2po8Q/Mozilla-Monitor-Premium-LP?node-id=3386-51938&t=R5Itups9WHRrAU3O-4
Description
Adds the “Free scan CTA” experiment. There are three variants of the landing page that we can run:
ctaWithEmail
: Email input with CTActaOnly
: Only the CTA without the email inputctaOnlyAlternativeLabel
: Only the CTA without the email input and a longer alternative button labelScreenshot
How to test
/
landing-page-free-scan-cta
innimbus.yaml
is enabled by default.variant
toctaWithEmail | ctaOnly | ctaOnlyAlternativeLabel
and rebuild the experiments.Checklist (Definition of Done)