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

MNTOR-3924: Plus Expiration heads-up page #5524

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Jan 21, 2025

References:

Jira: MNTOR-3924
Figma:

Description

Note: I did not add unit tests yet because it looks like Jest does not currently have access to the same version of React as our app does, and hence, useFormState is not supported:
https://stackoverflow.com/a/78736908

Once we upgrade to React 19 (and use useActionState instead), the tests can be added.

The same is also a requirement for showing the pending state, which requires the useActionState API.

It's also not finished yet because it's waiting on the back-end code to know which subscriptions are about to expire: #5512

However, since it's all under pretty tight timelines, I figured I'd submit this already to get a design and first code review, and finish it up once the other pieces are in place.

Screenshot (if applicable)

See Storybook, also for the terms.

How to test

I'm not sure if it's possible to test the coupon flow locally and if it's entirely done yet, but you should be able to view the individual pages in Storybook at least, and see the happy path and terms on the dev server.

Checklist (Definition of Done)

  • Localization strings (if needed) have been added.
  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in readme and/or code comments
  • I've added a unit test to test for potential regressions of this bug. - No, see above
  • If this PR implements a feature flag or experimentation, I've checked that it still works with the flag both on, and with the flag off.
  • If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Note: I did not add unit tests yet because it looks like Jest does
not currently have access to the same version of React as our app
does, and hence, useFormState is not supported:
https://stackoverflow.com/a/78736908

Once we upgrade to React 19 (and use useActionState instead), the
tests can be added.

The same is also a requirement for showing the pending state, which
requires the useActionState API.
@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Jan 21, 2025
@Vinnl Vinnl requested review from codemist and flozia January 21, 2025 15:53
@Vinnl Vinnl self-assigned this Jan 21, 2025
Copy link

@Vinnl Vinnl requested a review from flodolo as a code owner January 21, 2025 16:47
plus-expiration-error-already-applied-content-part2 = <support-link>Contact our Support team</support-link> for help or head to your dashboard to see the latest on how we’re protecting your data.
plus-expiration-error-already-applied-cta-label = Go to { -brand-monitor }
plus-expiration-terms-heading = <b>Terms and restrictions</b> 10% off subscription renewal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it just me or this string seems a bit strange? How does it look in the website?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Heh, now that you mention it it does, but it looked fine initially, and I think it's technically correct (though maybe needs a dash somewhere) - it's the terms for getting 10% off of the renewal of your subscription.

Here's what it looks like:

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it would make more sense swapped (10% off subscription renewal — <b>Terms and restrictions</b>).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll check with content.

@@ -74,6 +74,10 @@ const preview: Preview = {
linkTo("Pages/Public/Breach listing")();
}

if (path === "/terms/expiration-offer") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't look like this actually works anymore with the latest Storybook (I can't find the docs on it anymore either), but figured it couldn't harm in case they add it back or something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The line height of the h1 seems a bit too high as well as the indentation of the list. On mobile the content overlaps with the header logo.

image

<li>{l10n.getString("plus-expiration-terms-term5")}</li>
<li>
{l10n.getFragment("plus-expiration-terms-term6", {
elems: { "monitor-link": <Link href="/" /> },
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we don’t know the href value, yet: Should we add a TODO comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants