List breaches #2917

Vinnl · 2023-03-22T11:32:16Z

References:

Jira: MNTOR-1343 and MNTOR-1344
Figma: https://www.figma.com/file/4Cw0BfI1iqu0HzGkxPmsdU/Breach-Detail-Page?node-id=1-5385&t=ulhnif4UYxXluY85-4

Description

@rhelmer Most of the changes here are yours - I'm not sure if there were final touches you wanted to do, but if so, feel free to create a PR with just your changes, and I'll rebase on that.

This adds back the breach overview and breach details page to the refresh. Note that I did not add the background image on the overview - see this Figma comment.

Screenshot (if applicable)

How to test

Click the "Breaches" link in the header or footer. Search of a breached company, then click the card.

Checklist (Definition of Done)

Localization strings (if needed) have been added. - I think so, but possibly all strings have been given a do-over. I'm still checking this.
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. - N/A, I think, lacking UI testing infra
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.

Fixes linting issue in 84a4f62.

flodolo · 2023-03-22T12:45:31Z

locales/en/app.ftl

@@ -79,6 +79,9 @@ compromised-accounts = Compromised accounts:
 # compromised-data = the kind of user data exposed to hackers in data breach.
 compromised-data = Compromised data:

+# the kind of user data exposed to hackers in data breach.


Should we move these strings under the v2 section, together with the ones that are actually used? It might be worth checking with @toufali was his plans were.

Ah yes -- if you come across old strings that will be carried over, or add new strings it would be great to add/move them to the V2 section. Cleanup time is right around the corner and this will make our lives easier!

Going one step further, maybe all these strings should live in their own file, since this is a separate page and not pertinent to general site-wide strings, which I associate app.ftl with? all-breaches.ftl or similar? cc @flodolo since moving strings to another file is not insignificant.

I moved them to the V2 section in 068b0e7.

I was hesitant about moving them to a new file already, since I'm not sure if that will need alignment with the Pontoon folks, so I left that for now.

flodolo · 2023-03-22T12:46:29Z

locales/en/app.ftl

@@ -400,7 +404,8 @@ fb-comp-and-others =
 no-other-breaches-found = No other breaches found from a basic search.

 no-results-blurb = Sorry, that breach is not in our database.
-all-breaches-headline = All breaches in { -product-name }
+all-breaches-headline-2 = All breaches detected by { -product-name }


Suggested change

all-breaches-headline-2 = All breaches detected by { -product-name }

all-breaches-headline-2 = All breaches detected by { -brand-fx-monitor }

toufali

Great job getting this together in rapid fashion! I noted one blocker and a few other things that are less block-y, but somewhat important to me 😊

toufali · 2023-03-22T22:56:41Z

locales/en/app.ftl

@@ -79,6 +79,9 @@ compromised-accounts = Compromised accounts:
 # compromised-data = the kind of user data exposed to hackers in data breach.
 compromised-data = Compromised data:

+# the kind of user data exposed to hackers in data breach.


Ah yes -- if you come across old strings that will be carried over, or add new strings it would be great to add/move them to the V2 section. Cleanup time is right around the corner and this will make our lives easier!

Going one step further, maybe all these strings should live in their own file, since this is a separate page and not pertinent to general site-wide strings, which I associate app.ftl with? all-breaches.ftl or similar? cc @flodolo since moving strings to another file is not insignificant.

toufali · 2023-03-22T23:24:25Z

src/client/css/global.css

+@media (max-width: 480px) {
+  body > header menu a:not(.button) {
+    display: none;
+  }
+}


The app is set with a site-wide .mobile class that we might use here, instead of a media query:

Suggested change

@media (max-width: 480px) {

body > header menu a:not(.button) {

display: none;

}

}

.mobile body > header menu a:not(.button) {

display: none;

}

But going a step further, can we just do without the header "Breaches" link altogether?

it's not in the UX design and looks out-of-place to me

it seems redundant/confusing on the all-breaches index page since it just loads self

Assuming this is done to help a user back to the index after following a direct search link to a details page, it might be more intuitive to have a button (or two) on all details pages that reads something like "Go back to view all breaches" or just "View all breaches". Maybe one on top and one at the bottom?

Update, I posed this question to UX in Slack, thanks to urgent prodding by @pdehaan

Used the .mobile class in 818dd6b. The addition was discussed here, I'll keep an eye on your thread to remove this button in a follow-up PR if a conclusion isn't reached before this PR gets merged. (We can survive having the link there during Foxfooding, I presume :) )

Per UX I removed the header "All Breaches" link – it was also a fast-solve for the user-menu bug 🙌
f1bdd6d

src/client/js/search.js

toufali · 2023-03-22T23:42:10Z

src/client/css/partials/allBreachesPartial.css

Should we use the same file naming convention as the other files in the folder, and mirror the other related files in views/partials/ and client/js/partials/? i.e. this would become all-breaches.css

There's a lot of indirection going on, so it's a bit hard to follow, but if I understand it correctly, we automatically load CSS files whose file name, minus .css, matches the name of the function rendering the partial, so we can't use dashes there.

Yes you're right, and there's a bit of work I need to do to make this clearer. Unsurprisingly this will also likely land with code-splitting! I updated to better conform with existing naming conventions here: ffdcec9

With code-splitting, part of the task will be to use camelCase, so you were ahead of schedule 🚀

src/client/css/partials/breachDetailsPartial.css

src/views/guestLayout.js

src/utils/breach-logo.js

src/views/partials/all-breaches.js

src/utils/breach-logo.js

Partially reverts 89beb67.

They might also need to be moved to different files, but that might need coordination with Pontoon.

flodolo

Strings look good, please tag me again if there's new content

Vinnl · 2023-03-23T17:12:17Z

OK, content should be final now @flodolo, with some strings changed in 7ccc4ac.

That also removes the distinction between US and non-US strings for the VPN recommendation, as per Slack.

pdehaan · 2023-03-23T17:17:36Z

locales/en/app.ftl

+header-nav-all-breaches = All breaches
+footer-nav-all-breaches = All breaches


Why same string twice? Would the header and footer ever need to be translated differently?

As is often the resolution between the 3 of us, let's just remove the header link altogether and pretend this never happened?

Update: I removed the header link per UX, so this is non-issue. f1bdd6d

It is a recommendation from the l10n team (or at least Peiying) to not re-use strings. It's hardly an issue for localisers, since Pontoon will suggest the same translation, but different contexts might lead to different translations (or e.g. just changes in capitalisation) in some locales, so using different strings accommodates that.

Footer/Header is an edge case, but in general it's better to have different strings for different contexts.

src/utils/breach-logo.js

src/utils/recommendations.js

pdehaan · 2023-03-23T17:59:52Z

src/views/guestLayout.js

@@ -61,6 +62,7 @@ const guestLayout = data => `
        <img src='/images/moz-logo-1color-white-rgb-01.svg' width='100' height='29' loading='lazy' alt='${getMessage('mozilla')}'>
      </a>
      <menu>
+        <li><a href='/breaches'>${getMessage('footer-nav-all-breaches')}</a></li>
        <li><a href='https://support.mozilla.org/kb/firefox-monitor-faq' target='_blank'>FAQ</a></li>


Q: Does FAQ need to be translated?

git grep -n ">FAQ<" src | cat src/views/guestLayout.js:76: <li><a href='https://support.mozilla.org/kb/firefox-monitor-faq' target='_blank'>FAQ</a></li> src/views/mainLayout.js:110: <li><a href='https://support.mozilla.org/kb/firefox-monitor-faq' target='_blank'>FAQ</a></li>

Actually, just noticed this while grepping for a different PR on utm_* parameters:

git grep -n "https://support.mozilla.org/kb/firefox-monitor-faq" src | cat src/views/emails/email-2022.js:10: faq: 'https://support.mozilla.org/kb/firefox-monitor-faq', src/views/guestLayout.js:76: <li><a href='https://support.mozilla.org/kb/firefox-monitor-faq' target='_blank'>FAQ</a></li> src/views/mainLayout.js:105: <li><a href='https://support.mozilla.org/kb/firefox-monitor-faq' target='_blank'>FAQ</a></li> src/views/partials/landing.js:70: <a href='https://support.mozilla.org/kb/firefox-monitor-faq' target='_blank'>${getMessage('see-all-faq')}</a>

Where see-all-faq string is:

git grep -n "see-all-faq" locales/en/ | cat locales/en/landing.ftl:40:see-all-faq = See all FAQs

So looks like "FAQ" is hardcoded 2× and uses localized "See all FAQs" elsewhere.
Not necessarily a bug, but just something to confirm possibly.

But maybe that makes sense since they are used in the landing page vs footer:

Oh hm good catch, I'm not sure if we generally translate FAQ but I think it's a reasonable thing to do? Since this landed in 54ad4b6 and not the current PR would you mind filing a ticket for this? Thanks!

Done-ish. Filed as #2933. /cc @flodolo

Since it doesn't look like y'all use GitHub issues very much, I'll copy/pasta it to Jira and update this comment with the Jira bug number since that seems like the source of truth(tm).

(it should be translated, yes)

src/views/partials/breach-detail.js

Co-authored-by: Peter deHaan <pdehaan@hey.com>

…ozilla/blurts-server into MNTOR-1343-breach-page-styling

toufali · 2023-03-24T01:19:31Z

@flodolo minor string changes. Possibly most impactful: 72bf369

I know we discussed caps before, but I think this is ok? It's for a nav item in the footer. "Terms & Privacy" is also in the footer and title-case 🙏

toufali

I rush-pushed a bunch of stuff to get this over the finish line -- comments should be inline. Thanks for the team effort! 🎉

toufali · 2023-03-24T04:59:04Z

YOLO

rhelmer and others added 15 commits March 15, 2023 21:21

WIP

84a4f62

Clear search on reload, turn off autocomplete, remove debugging

5f2bda2

format dates

7a128ab

remove security tips per requirements

9d436bd

no need to modify the older client code

a2e1d0c

these pages do not use auth and do not need nonce

ec27adc

port the icons and recommendation sorter from pre-refresh version

89beb67

remove all inline style

b371194

join properly

c1ae307

simplify style

9222ea9

show CTAs correctly

3d6d120

recommendations may not be present

bde6e64

rebase to main and switch to new guestLayout

feb5785

Some styling for the breach overview page

f236864

fixup! WIP

587a62f

Vinnl self-assigned this Mar 22, 2023

Vinnl requested review from toufali and flodolo as code owners March 22, 2023 11:32

Vinnl added 3 commits March 22, 2023 12:38

Some styling for the breach detail page

5f5d6b1

Link to the breaches overview

c23f72a

fixup! WIP

6451c7e

Fixes linting issue in 84a4f62.

Vinnl force-pushed the MNTOR-1343-breach-page-styling branch from dda5803 to 6451c7e Compare March 22, 2023 11:38

flodolo requested changes Mar 22, 2023

View reviewed changes

fixup! Some styling for the breach overview page

3992e32

toufali requested changes Mar 23, 2023

View reviewed changes

Vinnl added 5 commits March 23, 2023 13:54

fixup! Link to the breaches overview

38c9c32

fixup! fixup! Link to the breaches overview

818dd6b

Move JS for allBreachesPartial into partials dir

b247e4c

Only run allBreachesPartial JS on the correct page

ef4802a

Remove unnecessary DOMContentLoaded listener

086ec77

Vinnl added 3 commits March 23, 2023 14:10

Remove mistakenly-removed header key

754522a

Partially reverts 89beb67.

Lazily load breach logos

a7f402f

Move some V2 strings into V2 section

068b0e7

They might also need to be moved to different files, but that might need coordination with Pontoon.

Vinnl requested review from flodolo and toufali March 23, 2023 13:22

Update some strings as per Content

9149545

flodolo approved these changes Mar 23, 2023

View reviewed changes

fixup! Update some strings as per Content

7ccc4ac

Vinnl requested a review from flodolo March 23, 2023 17:11

pdehaan reviewed Mar 23, 2023

View reviewed changes

toufali and others added 5 commits March 23, 2023 17:33

Update src/utils/recommendations.js

2883ece

Co-authored-by: Peter deHaan <pdehaan@hey.com>

Merge branch 'MNTOR-1343-breach-page-styling' of https://github.com/m…

4c886b2

…ozilla/blurts-server into MNTOR-1343-breach-page-styling

jsdoc type object for breach

a2654e9

remove header link and fix broken user menu

f1bdd6d

conform to partial naming convention

ffdcec9

mozilla deleted a comment from Faby666 Mar 24, 2023

toufali added 2 commits March 23, 2023 18:12

remove obsolete string

ae28dac

make "All Breaches" footer link title-case

72bf369

add "All Breaches" link to main layout footer

a07fafa

toufali approved these changes Mar 24, 2023

View reviewed changes

toufali added 2 commits March 23, 2023 21:22

remove 404 links to Security Tips

e2dc855

add/update utm to Breach Detail relay/vpn links

66fba1a

pdehaan mentioned this pull request Mar 24, 2023

Translate FAQ in v2 guestLayout and mainLayout? #2933

Open

fix search and avoid page reload on enter keypress

0fb5915

toufali merged commit 8a65954 into main Mar 24, 2023

toufali deleted the MNTOR-1343-breach-page-styling branch March 24, 2023 04:58

pdehaan mentioned this pull request May 25, 2023

Add layout for authenticated pages #3058

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List breaches #2917

List breaches #2917

Vinnl commented Mar 22, 2023 •

edited

Loading

flodolo Mar 22, 2023

toufali Mar 22, 2023 •

edited

Loading

Vinnl Mar 23, 2023

flodolo Mar 22, 2023

Vinnl Mar 22, 2023

toufali left a comment

toufali Mar 22, 2023 •

edited

Loading

toufali Mar 22, 2023 •

edited

Loading

toufali Mar 23, 2023

Vinnl Mar 23, 2023

toufali Mar 24, 2023

toufali Mar 22, 2023

Vinnl Mar 23, 2023

toufali Mar 24, 2023

flodolo left a comment

Vinnl commented Mar 23, 2023

pdehaan Mar 23, 2023

toufali Mar 23, 2023

toufali Mar 24, 2023

Vinnl Mar 24, 2023

flodolo Mar 24, 2023

pdehaan Mar 23, 2023 •

edited

Loading

pdehaan Mar 23, 2023

rhelmer Mar 24, 2023

pdehaan Mar 24, 2023

flodolo Mar 24, 2023

toufali commented Mar 24, 2023

toufali left a comment

toufali commented Mar 24, 2023

	all-breaches-headline-2 = All breaches detected by { -product-name }
	all-breaches-headline-2 = All breaches detected by { -brand-fx-monitor }

		header-nav-all-breaches = All breaches
		footer-nav-all-breaches = All breaches

List breaches #2917

List breaches #2917

Conversation

Vinnl commented Mar 22, 2023 • edited Loading

References:

Description

Screenshot (if applicable)

How to test

Checklist (Definition of Done)

Choose a reason for hiding this comment

toufali Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toufali left a comment

Choose a reason for hiding this comment

toufali Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

toufali Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flodolo left a comment

Choose a reason for hiding this comment

Vinnl commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdehaan Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toufali commented Mar 24, 2023

toufali left a comment

Choose a reason for hiding this comment

toufali commented Mar 24, 2023

Vinnl commented Mar 22, 2023 •

edited

Loading

toufali Mar 22, 2023 •

edited

Loading

toufali Mar 22, 2023 •

edited

Loading

toufali Mar 22, 2023 •

edited

Loading

pdehaan Mar 23, 2023 •

edited

Loading