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

How It Works Page - Mntor 2317 #4632

Merged
merged 97 commits into from
Jun 12, 2024
Merged

How It Works Page - Mntor 2317 #4632

merged 97 commits into from
Jun 12, 2024

Conversation

jespy2
Copy link
Contributor

@jespy2 jespy2 commented Jun 4, 2024

References:

Jira: MNTOR-2317
Figma: Multiple, see below

Description

As a potential firefox monitor user i would like to learn how the service works so I can decide if i would like to use it.

Designs: (use long edit) https://www.figma.com/file/YqpVpUbSTzZMj9Jbp2po8Q/Mozilla-Monitor-Premium-LP?type=design&node-id=1173-23054&mode=design&t=uIz7n6wwuYHNOgwZ-4

Screenshot (if applicable)

Screenshot 2024-06-04 at 12 17 37 PM

How to test

Once dev server is running, navigate to How It Works in your browser to see the page. Links to the page are described in the Jira ticket, which also need to be manually reviewed.

The new page and updated links have 100% test coverage, so run tests locally. Note: A test for WelcomeToPlus.test.tsx
(checks the How-It-Works page link) is failing. The test and the code being tested are almost identical to tests in LandingView.test.tsx, so the testing failing to find the element (a link) has been unresolved. I have been advised to go ahead and do this PR in case someone else on the team is able to find a solution.

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.
  • 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.

Comment on lines 128 to 144
<li className={styles.step}>
<div className={styles.stepTextContainer}>
<h3 className={`${styles.brokersEmphasis} ${styles.stepTitle}`}>
{l10n.getString("section-1-step-3-title")}
</h3>
<h3 className={styles.stepSubtitle}>
{l10n.getString("section-1-step-3-subtitle")}
</h3>
<p className={styles.stepBody}>
{l10n.getString("section-1-step-3-text-1")}
</p>
<p className={styles.stepBody}>
{l10n.getString("section-1-step-3-text-2")}
</p>
</div>
<Image src={RemoveStep3} alt="" />
</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: This may be a good example of how you can build a reusable component with props, promoting code modularity and avoiding repetition of the same li pattern. E.g. ResolutionContent.tsx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this suggestion, but the DataBreaches and DataBrokers have different styles in terms of background color and latter has an image for each step while the former has one image for three steps. I think it is doable, but am concerned that the time spent on this could delay launch. Would this be better as a tech debt item?


export const Footer = ({ l10n }: { l10n: ExtendedReactLocalization }) => {
export const Footer = ({
Copy link
Collaborator

@codemist codemist Jun 10, 2024

Choose a reason for hiding this comment

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

Landing Page:
image
How it works page (deploy preview):
image
Is there a reason we didn't just reuse this section from the landing page and why it looks a bit different on the HIW page? The former I feel is closer to the design spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Figma for this page has some differences from the landing page:
Screenshot 2024-06-10 at 10 41 05 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot is from the Figma page

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only difference I'm seeing is the HIBP attribution, which yes should be present for both pages.

Copy link
Collaborator

@codemist codemist left a comment

Choose a reason for hiding this comment

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

Really nice work! This was a significant first ticket to tackle, and you handled it very well with minimal friction. I hope the process was smooth for you, considering our coding patterns and standards. There is definitely a learning curve with factors like localization, showing/hiding elements based on geolocation, and styling libraries.

I left a few comments, but my biggest suggestion is to make the page look as close as possible to the design spec. Specifically, the first section could be improved by using the correct typefaces and sizes. Also, if you haven't already, I recommend doing a desk check with Tony and Jess using the deploy preview.

image image

@jespy2 jespy2 requested review from pdehaan and flozia June 10, 2024 15:20
@jespy2 jespy2 requested a review from codemist June 10, 2024 16:57
@codemist
Copy link
Collaborator

codemist commented Jun 10, 2024

For the header typefaces, could you use the tokens that use Inter as opposed to Metropolis so it looks closer to the design spec? And try to unify the "take control of your data" section with the landing page.

@codemist
Copy link
Collaborator

Also, on mobile there seems to be a lot of extra padding around this asset:
image

Comment on lines 11 to 13
.boldedText {
font-weight: 700;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: could we nest this in its respective parent class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here:
730612e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unable to comment on padding issue above. That is updated here:
9840f2c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unable to respond to comment on font family comment above.
Title text family update here:
854e4ae

Step titles converted to upper case update here:
b89852d

Copy link
Collaborator

@flozia flozia left a comment

Choose a reason for hiding this comment

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

Great job in connecting and working through all the requirements!

Comment on lines 93 to 97
placeholder={
props.placeholder
? props.placeholder
: l10n.getString("landing-all-hero-emailform-input-placeholder")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be just:

Suggested change
placeholder={
props.placeholder
? props.placeholder
: l10n.getString("landing-all-hero-emailform-input-placeholder")
}
placeholder={
props.placeholder ?? l10n.getString("landing-all-hero-emailform-input-placeholder")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion! Updated here:
ad609a1

@codemist codemist self-requested a review June 11, 2024 17:50
@jespy2 jespy2 merged commit 1f430c8 into main Jun 12, 2024
15 checks passed
@jespy2 jespy2 deleted the mntor-2317 branch June 12, 2024 14:14
Copy link

Cleanup completed - database 'blurts-server-pr-4632' destroyed, cloud run service 'blurts-server-pr-4632' destroyed

rhelmer added a commit that referenced this pull request Jun 12, 2024
rhelmer added a commit that referenced this pull request Jun 12, 2024
rhelmer added a commit that referenced this pull request Jun 12, 2024
@jespy2 jespy2 mentioned this pull request Jun 12, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants