-
Notifications
You must be signed in to change notification settings - Fork 223
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
Port breach-detail page to Next.js #3049
Conversation
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.
Looking good!
|
||
import Image, { StaticImageData } from "next/image"; | ||
import BreachDetailScanImage from "../../../../../client/images/breach-detail-scan.svg"; | ||
import "../../../../../client/css/partials/breachDetail.css"; |
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: I see 404
errors from breachDetail.css
since for the current Monitor site icons are being imported via background-image: url('/images/recommendation-icons/{iconName}.svg');
— not sure if we want to do anything with this right now?
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.
Oops, had to check those in: 4cbfa34
I just copied them over to /public/
, though I'm not super happy about the filenames having no indication of it being transitional (like /nextjs_migration/
), but 🤷
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.
Solid catch @flozia!
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx
Outdated
Show resolved
Hide resolved
<BreachLogo breach={breach} logos={breachLogos} /> | ||
{breach.Title} | ||
</h1> | ||
{getBreachCategory(breach) === "website-breach" ? ( |
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.
Note (non-blocking and non-actionable for now): I know the task right now is only porting the current pages we have. For the next iteration, it would be great if we would define utilize enums more and minimize string comparisons.
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 I might actually push back a little on enums: I believe the TS team wouldn't have added those today, since they're a language-level feature added to JS, rather than just being annotations - and IIRC they're even incompatible with the enum proposal that's on the TC39 standardisation track.
That said, I do agree in spirit: I just think that string comparisons are fine, but would also like to tighten the type to a union of string literals.
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.
@Vinnl To confirm your intent, it would only return one of three possible values?
"data-aggregator-breach" | "website-breach" | "sensitive-breach"
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 scraped each breach (because of course I would) and used cheerio
to extract .breach-detail-meta-more-info
on each breach detail's page into a Set()
and it says we only have 3 unique breach types:
Set(3) {
'Website Breach',
'Data Aggregator Breach',
'Sensitive Website Breach'
}
Now that we're using TypeScript, I'd probably suggest moving those into an enum since it looks like they are hardcoded in a few places:
git grep -En "['\"](data-aggregator|website|sensitive)-breach['\"]" -- ':!*.ftl' | cat
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx:72: {getBreachCategory(breach) === "website-breach" ? (
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx:194: if (categoryId === "data-aggregator-breach") {
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx:199: } else if (categoryId === "sensitive-breach") {
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx:234: return "data-aggregator-breach";
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx:237: return "sensitive-breach";
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx:240: return "website-breach";
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx:242: return "data-aggregator-breach";
src/views/partials/breachDetail.js:11: return 'data-aggregator-breach'
src/views/partials/breachDetail.js:14: return 'sensitive-breach'
src/views/partials/breachDetail.js:17: return 'website-breach'
src/views/partials/breachDetail.js:19: return 'data-aggregator-breach'
src/views/partials/breachDetail.js:103: if (categoryId === 'data-aggregator-breach') {
src/views/partials/breachDetail.js:109: } else if (categoryId === 'sensitive-breach') {
src/views/partials/breachDetail.js:138: ${getBreachCategory(data.breach) === 'website-breach'
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.
@maxxcrawford Yep exactly, and TS would flip us off if we tried to use any other!
import Glyph1 from "../../../../../client/images/social-security-numbers.svg"; | ||
import Glyph3 from "../../../../../client/images/passwords.svg"; |
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.
No Glyph2?
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.
Heh, this was me trying to efficiently copy-paste the filenames from a different file and then converting that to these imports - the other file had the SSN file listed twice. Probably a good idea to update the numbers yeah, will take a look tomorrow 😅
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.
suggestion: I would make these more logically named.
social-security-numbers: glyphSocialSecurityNumbers
, etc.
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.
Dammit Maxx, you know how much manual typing this was already? :P
But you're right and I'm lazy, of course, so fixed in ebd583e.
* License, v. 2.0. If a copy of the MPL was not distributed with this | ||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
import Image, { StaticImageData } from "next/image"; |
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.
non-blocking: Interesting that we're using semicolons here but not in the .js files.
Makes me wonder if we're linting .ts|.tsx files yet. If not, we should probably set that up sooner rather than later so we don't accrue a lot of ESLint-tsx-tech-debt.
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.
Yeah, we agreed to switch to Prettier for formatting (it's more complete, and doesn't show up as warnings before it's been formatting), since we're creating many new files anyway. So TS files are formatted by Prettier, while existing files are left as-is.
When the migration is complete, we might still want to do a mega-formatting commit for the remaining files.
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.
Filed #3055
<a | ||
href="#what-is-this-breach" | ||
className="breach-detail-meta-more-info" | ||
> | ||
{l10n.getString(getBreachCategory(breach))} | ||
</a> |
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.
Re: internal page links, should we be using <a>
or Next.js's <Link>
component?
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.
@Vinnl can add to this, but as this is just a port, and not a true React page, we're maintaining <a>
for now.
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.
Yeap, that's exactly it - we're reusing the existing code, which is expecting all client-side state to be thrown away on page navigations, so using <Link>
here would be unsafe. We should definitely do so for new pages though, once we're going to actually use React.
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.
@Vinnl Suggestion: We may want to document how we're using (not using?) <Link>
for the porting pages task.
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.
Added, thanks!
</ul> | ||
<p | ||
className="breach-detail-attribution" | ||
dangerouslySetInnerHTML={{ |
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.
Are we 'dangerous' because the HIBP link has a nested <a>
tag?
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.
@pdehaan Yeah. The other thing to call out is that we're already doing this, but React surfaces this dangerous act better.
Note that we should be able to migrate away from this in the future, changing how we set up strings in React. (Similar to Relay) so there's no mockup/HTML content in the .ftl file
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.
Also note that the danger lies in the source strings containing malicious code - but since those source strings are provided by the localisers, that should be fine.
priority: [] as any[], | ||
lowerPriority: [] as any[], | ||
}; | ||
|
||
const dataClasses: any[] = breach.DataClasses; |
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.
nit: Not sure if we can use anything more specific than any
here.
Not sure if they're all strings. 🤷
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.
Yeah, ideally we wouldn't have to set a type for this value in the first place - #3011 will hopefully get us closer to that.
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.
@Vinnl Good reminder. I'll get that merged in without checking anything, for future usage.
src/app/(nextjs_migration)/(guest)/breach-details/[breachName]/page.tsx
Outdated
Show resolved
Hide resolved
|
||
return ( | ||
<> | ||
{output} {lastOutputItem} |
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.
Q: Do we need to check if there are "More" items? Wondering if we have any risk of a trailing "+" icon without any data classes.
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.
https://monitor.firefox.com/breach-details/126
In fact, I found 69/673 (10.25%) breaches without "more" data classes.
import CachedFetch from "@11ty/eleventy-fetch";
import * as cheerio from "cheerio";
const noMas = [];
const breaches = await getBreaches();
for (const breach of breaches) {
const $ = await getBreachPage(breach.Name);
const more = $("ul.breach-detail-compromised-list li:last-of-type").text().trim();
if (!more) {
noMas.push(breach.Name);
}
}
console.log(`${noMas.length}/${breaches.length} breaches without "more" data classes`);
console.log(noMas);
async function getBreaches() {
return CachedFetch("https://haveibeenpwned.com/api/v3/breaches", {type: "json", duration: "1d"});
}
async function getBreachPage(breachName) {
const res = await CachedFetch(`https://monitor.firefox.com/breach-details/${breachName}`, { type: "text", duration: "1d" });
return cheerio.load(res);
}
Obviously, not a blocker since this is already in production.
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.
Of course you did 😄 Well, might as well fix it now: d6367ef
Stylelint is having feelings: 353 problems (349 errors, 4 warnings)
Looks like we're explicitly overriding this in our .stylelintrc at Lines 27 to 32 in 4cbfa34
Removing that rule gives us the following output: 10 problems (6 errors, 4 warnings) |
As for the ESLint errors, they're a bit lost in the noise (although I reported this at #3035, so no point in rehashing here):
Here's the errors (and a pretty gross chain of npm run lint:js | grep -Ev "\d+:\d+\s+warning\s+" | grep -B1 -E "\serror\s"
/Volumes/Dev/github/mozilla/blurts-server/src/client/js/dialog.js
64:7 error Do not assign to the variable `module`. See: https://nextjs.org/docs/messages/no-assign-module-variable @next/next/no-assign-module-variable
--
/Volumes/Dev/github/mozilla/blurts-server/src/client/js/partials/exposuresSetup.js
58:15 error Empty block statement no-empty
--
/Volumes/Dev/github/mozilla/blurts-server/src/controllers/dialog.js
17:5 error Do not assign to the variable `module`. See: https://nextjs.org/docs/messages/no-assign-module-variable @next/next/no-assign-module-variable
--
/Volumes/Dev/github/mozilla/blurts-server/src/e2e/utils/helpers.js
91:11 error Empty block statement no-empty
--
/Volumes/Dev/github/mozilla/blurts-server/src/utils/email.test.js
121:5 error Definition for rule 'n/no-callback-literal' was not found n/no-callback-literal Or if I manually disable a bunch of ESLint rules (re:JSDoc, etc): > eslint .
/Volumes/Dev/github/mozilla/blurts-server/next-env.d.ts
6:1 warning header should be a block comment header/header
/Volumes/Dev/github/mozilla/blurts-server/src/app/components/server/BreachLogo.tsx
22:12 warning Using `<img>` could result in slower LCP and higher bandwidth. Consider using `<Image />` from `next/image` to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element @next/next/no-img-element
/Volumes/Dev/github/mozilla/blurts-server/src/client/js/dialog.js
64:7 error Do not assign to the variable `module`. See: https://nextjs.org/docs/messages/no-assign-module-variable @next/next/no-assign-module-variable
/Volumes/Dev/github/mozilla/blurts-server/src/client/js/partials/exposuresSetup.js
58:15 error Empty block statement no-empty
/Volumes/Dev/github/mozilla/blurts-server/src/controllers/dialog.js
17:5 error Do not assign to the variable `module`. See: https://nextjs.org/docs/messages/no-assign-module-variable @next/next/no-assign-module-variable
/Volumes/Dev/github/mozilla/blurts-server/src/db/tables/subscribers_types.d.ts
3:1 warning header should be a block comment header/header
/Volumes/Dev/github/mozilla/blurts-server/src/e2e/utils/helpers.js
91:11 error Empty block statement no-empty
/Volumes/Dev/github/mozilla/blurts-server/src/utils/email.test.js
121:5 error Definition for rule 'n/no-callback-literal' was not found n/no-callback-literal
✖ 8 problems (5 errors, 3 warnings) Still a bit unclear on "n/no-callback-literal". Looks like this was always |
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.
Mostly questions/comments on the current code. Thanks again for walking us through this process, @Vinnl! 🎉
* License, v. 2.0. If a copy of the MPL was not distributed with this | ||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
import Image, { StaticImageData } from "next/image"; |
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.
Filed #3055
|
||
import Image, { StaticImageData } from "next/image"; | ||
import BreachDetailScanImage from "../../../../../client/images/breach-detail-scan.svg"; | ||
import "../../../../../client/css/partials/breachDetail.css"; |
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.
Solid catch @flozia!
import Glyph1 from "../../../../../client/images/social-security-numbers.svg"; | ||
import Glyph3 from "../../../../../client/images/passwords.svg"; |
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.
suggestion: I would make these more logically named.
social-security-numbers: glyphSocialSecurityNumbers
, etc.
import Glyph1 from "../../../../../client/images/social-security-numbers.svg"; | ||
import Glyph3 from "../../../../../client/images/passwords.svg"; | ||
import Glyph4 from "../../../../../client/images/bank-account-numbers.svg"; | ||
import Glyph5 from "../../../../../client/images/credit-cards.svg"; | ||
import Glyph6 from "../../../../../client/images/credit-card-cvvs.svg"; | ||
import Glyph7 from "../../../../../client/images/partial-credit-card-data.svg"; | ||
import Glyph8 from "../../../../../client/images/ip-addresses.svg"; | ||
import Glyph9 from "../../../../../client/images/historical-passwords.svg"; | ||
import Glyph10 from "../../../../../client/images/security-questions-and-answers.svg"; | ||
import Glyph11 from "../../../../../client/images/phone-numbers.svg"; | ||
import Glyph12 from "../../../../../client/images/email-addresses.svg"; | ||
import Glyph13 from "../../../../../client/images/dates-of-birth.svg"; | ||
import Glyph14 from "../../../../../client/images/pins.svg"; | ||
import Glyph15 from "../../../../../client/images/physical-addresses.svg"; | ||
import MoreIcon from "../../../../../client/images/more.svg"; | ||
|
||
const glyphs: Record<string, StaticImageData> = { | ||
"social-security-numbers": Glyph1, | ||
passwords: Glyph3, | ||
"bank-account-numbers": Glyph4, | ||
"credit-cards": Glyph5, | ||
"credit-card-cvvs": Glyph6, | ||
"partial-credit-card-data": Glyph7, | ||
"ip-addresses": Glyph8, | ||
"historical-passwords": Glyph9, | ||
"security-questions-and-answers": Glyph10, | ||
"phone-numbers": Glyph11, | ||
"email-addresses": Glyph12, | ||
"dates-of-birth": Glyph13, | ||
pins: Glyph14, | ||
"physical-addresses": Glyph15, | ||
}; |
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.
Suggested from above comment:
I converted everything to camelCase with a leading glyph
. Hopefully this helps?
import Glyph1 from "../../../../../client/images/social-security-numbers.svg"; | |
import Glyph3 from "../../../../../client/images/passwords.svg"; | |
import Glyph4 from "../../../../../client/images/bank-account-numbers.svg"; | |
import Glyph5 from "../../../../../client/images/credit-cards.svg"; | |
import Glyph6 from "../../../../../client/images/credit-card-cvvs.svg"; | |
import Glyph7 from "../../../../../client/images/partial-credit-card-data.svg"; | |
import Glyph8 from "../../../../../client/images/ip-addresses.svg"; | |
import Glyph9 from "../../../../../client/images/historical-passwords.svg"; | |
import Glyph10 from "../../../../../client/images/security-questions-and-answers.svg"; | |
import Glyph11 from "../../../../../client/images/phone-numbers.svg"; | |
import Glyph12 from "../../../../../client/images/email-addresses.svg"; | |
import Glyph13 from "../../../../../client/images/dates-of-birth.svg"; | |
import Glyph14 from "../../../../../client/images/pins.svg"; | |
import Glyph15 from "../../../../../client/images/physical-addresses.svg"; | |
import MoreIcon from "../../../../../client/images/more.svg"; | |
const glyphs: Record<string, StaticImageData> = { | |
"social-security-numbers": Glyph1, | |
passwords: Glyph3, | |
"bank-account-numbers": Glyph4, | |
"credit-cards": Glyph5, | |
"credit-card-cvvs": Glyph6, | |
"partial-credit-card-data": Glyph7, | |
"ip-addresses": Glyph8, | |
"historical-passwords": Glyph9, | |
"security-questions-and-answers": Glyph10, | |
"phone-numbers": Glyph11, | |
"email-addresses": Glyph12, | |
"dates-of-birth": Glyph13, | |
pins: Glyph14, | |
"physical-addresses": Glyph15, | |
}; | |
import glyphSocialSecurityNumbers from "../../../../../client/images/social-security-numbers.svg"; | |
import glyphPasswords from "../../../../../client/images/passwords.svg"; | |
import glyphBankAccountNumbers from "../../../../../client/images/bank-account-numbers.svg"; | |
import glyphCreditCards from "../../../../../client/images/credit-cards.svg"; | |
import glyphCreditCardCvvs from "../../../../../client/images/credit-card-cvvs.svg"; | |
import glyphPartialCreditCardData from "../../../../../client/images/partial-credit-card-data.svg"; | |
import glyphIpAddresses from "../../../../../client/images/ip-addresses.svg"; | |
import glyphHistoricalPasswords from "../../../../../client/images/historical-passwords.svg"; | |
import glyphSecurityQuestionsAndAnswers from "../../../../../client/images/security-questions-and-answers.svg"; | |
import glyphPhoneNumbers from "../../../../../client/images/phone-numbers.svg"; | |
import glyphEmailAddresses from "../../../../../client/images/email-addresses.svg"; | |
import glyphDatesOfBirth from "../../../../../client/images/dates-of-birth.svg"; | |
import glyphPins from "../../../../../client/images/pins.svg"; | |
import glyphPhysicalAddresses from "../../../../../client/images/physical-addresses.svg"; | |
import MoreIcon from "../../../../../client/images/more.svg"; | |
const glyphs: Record<string, StaticImageData> = { | |
"social-security-numbers": glyphSocialSecurityNumbers, | |
passwords: glyphPasswords, | |
"bank-account-numbers": glyphBankAccountNumbers, | |
"credit-cards": glyphCreditCards, | |
"credit-card-cvvs": glyphCreditCardCvvs, | |
"partial-credit-card-data": glyphPartialCreditCardData, | |
"ip-addresses": glyphIpAddresses, | |
"historical-passwords": glyphHistoricalPasswords, | |
"security-questions-and-answers": glyphSecurityQuestionsAndAnswers, | |
"phone-numbers": glyphPhoneNumbers, | |
"email-addresses": glyphEmailAddresses, | |
"dates-of-birth": glyphDatesOfBirth, | |
pins: glyphPins, | |
"physical-addresses": glyphPhysicalAddresses | |
}; |
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.
nit: re: @maxxcrawford's comment/suggestion above, change from "MoreIcon" to "glyphMore" for consistency?
-import MoreIcon from "../../../../../client/images/more.svg";
+import glyphMore from "../../../../../client/images/more.svg";
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.
Aaargh, of course I only saw this after having done the rename myself 🤦
Although I'm OK with camelCase instead of PasacalCase: ebd583e. Also, I just realised I don't even really know what "glyph" means.
<BreachLogo breach={breach} logos={breachLogos} /> | ||
{breach.Title} | ||
</h1> | ||
{getBreachCategory(breach) === "website-breach" ? ( |
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.
@Vinnl To confirm your intent, it would only return one of three possible values?
"data-aggregator-breach" | "website-breach" | "sensitive-breach"
<a | ||
href="#what-is-this-breach" | ||
className="breach-detail-meta-more-info" | ||
> | ||
{l10n.getString(getBreachCategory(breach))} | ||
</a> |
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.
@Vinnl Suggestion: We may want to document how we're using (not using?) <Link>
for the porting pages task.
</div> | ||
</div> | ||
|
||
{/* Sign Up Banner */} |
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.
nit/random question: Do we want to use a different style of comments here?
{/* Sign Up Banner */} | |
{ | |
// Sign Up Banner | |
} |
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 don't have a strong preference, just went with this style because that's what VSCode gives you if you press Ctrl+/ :) Switched in 18f213a
"Exactis", | ||
"Apollo", | ||
"YouveBeenScraped", | ||
"ElasticsearchSalesLeads", | ||
"Estonia", | ||
"MasterDeeds", | ||
"PDL", |
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.
nit/suggestion: Move this to a variable for legibility. Right now, it's a loose array of breach names where the context is only inferred by the if statement logic.
const dataAggregatorBreach = ["Exactis", "Apollo", "YouveBeenScraped", "ElasticsearchSalesLeads", "Estonia", "MasterDeeds", "PDL"]
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.
Good suggestion: 1b3818c
priority: [] as any[], | ||
lowerPriority: [] as any[], | ||
}; | ||
|
||
const dataClasses: any[] = breach.DataClasses; |
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.
@Vinnl Good reminder. I'll get that merged in without checking anything, for future usage.
<dt>{l10n.getString(r.recommendationCopy.subhead)}</dt> | ||
<dd> | ||
<p>{l10n.getString(r.recommendationCopy.body)}</p> | ||
{r.recommendationCopy.cta ? ( | ||
<a | ||
href={r.ctaHref} | ||
target={r.ctaShouldOpenInNewTab ? "_blank" : "_self"} | ||
> | ||
{l10n.getString(r.recommendationCopy.cta)} | ||
</a> | ||
) : null} | ||
</dd> |
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.
nit/question: Should there be an encapsulating <dl>
for these?
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.
There absolutely should be. And there was, but it's hard to find with all the indirection going on; I moved it closer in ea5d8dc.
ea5d8dc
to
2c4e58f
Compare
References:
Jira: MNTOR-1637
Didn't change much after our pairing session; just fixed up the recommendation icons.