-
Notifications
You must be signed in to change notification settings - Fork 200
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
MNTOR-3057 - Cancel flow discount callout #4635
Conversation
Preview URL 🚀 : https://blurts-server-pr-4635-mgjlpikfea-uk.a.run.app |
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.
Overall looking good to me, tests are breaking due to the console logs I believe.
I haven't had the chance to manually test the UI e2e, will do
src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/settings/CancelFlow.tsx
Outdated
Show resolved
Hide resolved
src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/settings/actions.ts
Outdated
Show resolved
Hide resolved
|
||
const discountedNext3Months: DiscountData = { | ||
headline: l10n.getString("settings-unsubscribe-dialog-promotion-cta", { | ||
discount_percentage_num: "30%", |
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.
Question: Is it OK to pass the %
sign here or should this be part of the localized string?
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.
@flodolo ?
const [primaryCta, setPrimaryCta] = useState<ReactElement>(); | ||
const [ctaSubtitle, setCtaSubtitle] = useState<ReactElement>(); |
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.
React component should not go into state
: What Shouldn’t Go in State?. Since the entire component rerenders when you update step
with setStep
you can just assign primaryCta
and ctaSubtitle
to a variable depending on the current step.
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.
Ah gotcha! I previously used state
because in the old nimbus experiments implementation it seemed like we wanted to account for different variations of the discount code step. Good to know though. 06d0d4b
// Current experiment | ||
// Note: Opted to use state here to support future experiments |
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.
As mentioned above, I don’t think you would need to store the elements in state.
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.
Fixed in 06d0d4b
/> | ||
</video> | ||
{/* Fall back to the image if the video formats are not supported: */} | ||
{/* The .staticAlternative class ensures that this image will only be shown if the user has prefers-reduced-motion on */} |
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.
Praise: Nice!
.goToDashboardCta { | ||
margin-top: $spacing-lg; | ||
font: $text-body-md; | ||
font-weight: 500; | ||
border: 0; | ||
padding: $spacing-md $spacing-lg; | ||
border-radius: $border-radius-md; | ||
cursor: pointer; | ||
display: inline-block; | ||
line-height: 1; | ||
text-align: center; | ||
text-decoration: none; | ||
color: $color-white; | ||
background-color: $color-blue-50; | ||
|
||
&:hover { | ||
background-color: $color-blue-60; | ||
} | ||
|
||
&:focus { | ||
outline: $border-focus-width solid $color-blue-30; | ||
} | ||
} | ||
|
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'd use the styling from Button
here but alas, "Go to dashboard" functions as an internal link and I had to use the TelemetryLink
component which does not use the Button
, hence the custom styling.
className={` | ||
${styles.cancellationIllustrationWrapper} | ||
${styles.staticAlternative} | ||
`} |
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.
Formatting: Not sure why the linter is not picking up on this, but the formatting seems to be off here?
|
||
const checkCouponCode = async () => { | ||
const result = await onCheckUserHasCurrentCouponSet(); | ||
if (typeof result.success === "boolean") { |
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.
Just checking for a boolean here might not be enough. It’s safer to explicitly check if result.success
is also truthy.
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.
Thanks for the fix, lgtm on the calls
/* c8 ignore start */ | ||
const handleApplyCouponCode = async () => { | ||
const result = await onApplyCouponCode(); | ||
if (result?.success) { |
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 this can be simplified as: setCouponSuccess(!!result?.success);
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--no additional comments
Cleanup completed - database 'blurts-server-pr-4635' destroyed, cloud run service 'blurts-server-pr-4635' destroyed |
References:
Jira: MNTOR-3057
Figma: https://www.figma.com/design/DTbmXzCQCw2PxXpHQW8yfW/Monitor-MVP-Enhancements?node-id=1157-25611&t=CyXrO2sT6Vra9psg-0
Description
The flags
CancellationFlow
,ConfirmCancellation
andDiscountCouponNextThreeMonths
are required. You'll also need theCURRENT_COUPON_CODE_ID
env var set, which can be found in this ticket: https://mozilla-hub.atlassian.net/browse/MNTOR-3057?focusedCommentId=895742.With account that's subscribed to premium, go through the cancellation flow and the 30% off discount state should be visible. Selecting the CTA should output a
fxa_apply_coupon_code_success
in your server logs, and you should be taken to theall-set
dialog state.Screenshot (if applicable)
Screen.Recording.2024-06-13.at.3.34.02.PM.mov
How to test
Checklist (Definition of Done)