-
Notifications
You must be signed in to change notification settings - Fork 207
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-1692 Resolution Flow: Data Broker Results #3201
MNTOR-1692 Resolution Flow: Data Broker Results #3201
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.
very review? but @maxxcrawford said "DO NOT REVIEW"!!! :ohno: |
d6efa0c
to
c80aa41
Compare
.../redesign/(authenticated)/user/dashboard/fix/data-broker-profiles/view-data-brokers/page.tsx
Outdated
Show resolved
Hide resolved
|
||
return ( | ||
<button className={styles.navArrowBack} onClick={() => router.back()}> | ||
<Image alt="" src={imageNavLeft} /> |
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: might need a localized "Back" alt/aria-label for screen readers.
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: This comment still stands, even if it's not technically going "back" in history, but more to the previous step.
de5db3b
to
4df524a
Compare
locales-pending/fix.ftl
Outdated
fix-flow-data-broker-profiles-view-data-broker-profiles-button-view-less = View less |
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.
nits: add linebreak and remove trailing spaces.
{l10n.getString( | ||
"fix-flow-data-broker-profiles-view-data-broker-profiles-view-info-on-sites" | ||
)} | ||
<button> | ||
<button className={styles.questionTooltip}> |
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.
Probably off topic, but I think the <button>
or <Image>
tags needs some a11y alt=
or aria-label=
attributes for screen readers.
margin: 0; | ||
padding: 0; | ||
|
||
&:nth-child(n + 5) { |
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: add comment on what element nth-child(n + 5)
is referring to.
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 a context comment to every (n +...)
selector. Revised in 67ecbc1
import { useL10n } from "../../../../../../../../hooks/l10n"; | ||
import { DataBrokerProfiles } from "../../../../../../../../components/client/DataBrokerProfiles"; | ||
import iconQuestionMark from "./images/icon-question-mark.svg"; | ||
import { getOnerepScanResults } from "../../../../../../../../../db/tables/onerep_scans"; |
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.
./src/app/(proper_react)/redesign/(authenticated)/user/dashboard/fix/data-broker-profiles/view-data-brokers/page.tsx
15:10 Warning: 'getOnerepScanResults' is defined but never used. Allowed unused vars must match /^_/u. @typescript-eslint/no-unused-vars
import Image from "next/image"; | ||
import { usePathname } from "next/navigation"; | ||
import styles from "./FixNavigation.module.scss"; | ||
import stepDoneIcon from "./assets/step-counter-done.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.
10:8 Warning: 'stepDoneIcon' is defined but never used. Allowed unused vars must match /^_/u. @typescript-eslint/no-unused-vars
|
||
// type NavigationItems = Array<NavigationItemsContent> | ||
|
||
export const FixNavigation = (props: Props) => { |
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.
38:31 Warning: 'props' is defined but never used. Allowed unused args must match /^_/u. @typescript-eslint/no-unused-vars
Also unclear on what "Props" is here since the definition above on L25 is commented out (yet ESLint didn't seemingly flag that as an issue?)
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 just noticed the squiggle line. Same thought - it's weird lint didn't catch this? Added a props definition. Revised in 35e256b
); | ||
}; | ||
|
||
function isStepDone(step: StepId, currentStep: StepId): 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.
233:10 Warning: 'isStepDone' is defined but never used. Allowed unused vars must match /^_/u. @typescript-eslint/no-unused-vars
0709a4a
to
60a0f8f
Compare
60a0f8f
to
f11e432
Compare
locales-pending/fix.ftl
Outdated
fix-flow-nav-leaked-passwords = Leaked passwords | ||
fix-flow-nav-security-recommendations = Security recommendations | ||
fix-flow-data-broker-profiles-start-free-scan-headline = Find out which sites are selling your personal info | ||
fix-flow-data-broker-profiles-start-free-scan-content-p1 = Before you review your data breaches, would you like us to search for you on 190 data broker sites that may be selling your personal information? |
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: we should make 190
a variable, so we can optionally update across n locales from a single source.
It seems to be hardcoded here and elsewhere in the .ts/.tsx templates.
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 prefer not to do that, because then it becomes a huge pain to translate (6 variants for some locales). Honestly, better to be more vague, e.g. "almost 200 hundred", because nobody will go and keep this up to date with the real number…
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.
In this case, I think hundreds
would work just as well.
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.
BTW, looks like we used the plural approach on another string :-(
onboarding-get-started-how-it-works-dialog-step1-content = {
$dataBrokerTotalCount ->
*[other] With just a few key pieces of information, we’ll search for you in all known data breaches and { $dataBrokerTotalCount } major data broker sites. All users get the first scan free.
}
Can we ask content to revisit this, and avoid shooting ourselves in the foot by putting a number that needs to be kept up-to-date? Based on previous experiences, we'll just forget to update it anyway, and this creates extra-work for some languages that is not completely warratend.
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.
This isn't scientific or probably a great grep(tm), but possibly interesting:
git grep -IPn "\b190\b" -- ':!*.svg' | cat
.env-dist:78:ONEREP_DATA_BROKER_COUNT=190
locales-pending/fix.ftl:8:fix-flow-data-broker-profiles-start-free-scan-content-p1 = Before you review your data breaches, would you like us to search for you on 190 data broker sites that may be selling your personal information?
locales-pending/fix.ftl:41:# $data_broker_count is the number of data brokers scanned monthly (As of adding this, it is 190). This number will always be plural.
locales-pending/fix.ftl:59:# $data_broker_count is the number of data brokers scanned monthly (As of adding this, it is 190). This number will always be plural.
locales-pending/onboarding.ftl:16:# $dataBrokerTotalCount (number) - number of scanned data broker sites, e.g. 190
locales-pending/onboarding.ftl:62:# $dataBrokerTotalCount (number) - number of scanned data broker sites, e.g. 190
src/app/(proper_react)/redesign/(authenticated)/user/dashboard/DashboardTopBanner.tsx:52: data_broker_sites_total_num: 190,
src/app/(proper_react)/redesign/(authenticated)/user/dashboard/DashboardTopBanner.tsx:69: data_broker_sites_total_num: 190,
src/app/(proper_react)/redesign/(authenticated)/user/dashboard/fix/data-broker-profiles/automatic-remove/page.tsx:30: data_broker_count: 190,
src/app/(proper_react)/redesign/(authenticated)/user/dashboard/fix/data-broker-profiles/automatic-remove/page.tsx:76: data_broker_count: 190,
src/app/(proper_react)/redesign/(authenticated)/user/welcome/Onboarding.stories.tsx:20: dataBrokerCount={190}
src/app/components/client/ExposuresFilterExplainer.tsx:30: data_broker_sites_total_num: 190,
src/app/components/client/ExposuresFilterExplainer.tsx:61: data_broker_sites_total_num: 190,
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 @pdehaan already found out: We are setting ONEREP_DATA_BROKER_COUNT
as an env variable. It’s not only used for strings, but also calculations on views. I’d expect that variable to not change often. If it does, it will be updated. Nevertheless, if this is still a localization concern because of the additional effort this creates we’ll need to sync with PM & Content.
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.
@flozia How do you know it's 190?
I found https://onerep.com/sites-we-remove-from and if I copy-paste+merge+dedupe the "People-search sites" and "Mirror people-search sites" sections, I get 195 results. But not sure if you're using some API or better method of calculating. I guess we could just have a monthly/quarterly calendar reminder or something if it truly is a manual task to count up results and keep them in sync.
Ah, OK, maybe they do have an API for this: https://docs.onerep.com/#tag/Data-Brokers
But I can't ping it directly since I don't have an API key.
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 This is a side-note, but to help lock in the exact number, we're discussing/determining how to support individual logos for each data broker.
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 The manual logo team is back, baby! :r+:
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.
@flozia @pdehaan As stated else where in this review, I am using the plural fluent format *[other] {...
and referencing the environment variable.
One note for @flozia: I ran into a hydration error issue when doing this, so it's still hard-coded, but I added a FIXME comment regarding it for now. (See 6e660ee)
{l10n.getString( | ||
"fix-flow-data-broker-profiles-start-free-scan-content-p2" | ||
)} | ||
<a href="#"> |
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.
TODO: Add href
? ALso, should this use <Link>
?
)} | ||
</Link> | ||
<Link | ||
className="button secondary" |
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.
Why is it
className="button secondary"
here, but
className={`${buttonStyles.button} ${buttonStyles.primary}`}
above?
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.
Why? 🤷
But hey! Great catch. Revised in 7e7a9d0
import imageArrowLeft from "./images/icon-arrow-left.svg"; | ||
import imageArrowRight from "./images/icon-arrow-right.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.
nit: elsewhere we name these Pascal case ImageArrowLeft
since they're a DOM Element.
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.
Ahhh! Nice callout. Revised in 7e7a9d0
{ | ||
key: "data-broker-profiles", | ||
labelStringId: "fix-flow-nav-data-broker-profiles", | ||
name: "Data broker profiles", |
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: Are these name
s user facing and need to be localized?
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.
Interesting. This attribute is no longer being used (in favor of the labelStringId
entry which does reference a localized string). Removed in 7e7a9d0
></div> | ||
{/* TODO: Add logic to show unique image per data broker */} | ||
{/* <Image src={} alt={props.data.data_broker} /> */} | ||
<a href={props.data.url}> |
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: Is this a local URL, and therefore this should be a <Link>
element?
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.
Coincidentally, I think you want props.data.link
here, not props.data.url
. Per example output in the API, url
is a pointer to the user's profile on OneRep's site, link
is a link to the data broker.
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.
locales-pending/fix.ftl
Outdated
fix-flow-nav-leaked-passwords = Leaked passwords | ||
fix-flow-nav-security-recommendations = Security recommendations | ||
fix-flow-data-broker-profiles-start-free-scan-headline = Find out which sites are selling your personal info | ||
fix-flow-data-broker-profiles-start-free-scan-content-p1 = Before you review your data breaches, would you like us to search for you on 190 data broker sites that may be selling your personal information? |
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.
In this case, I think hundreds
would work just as well.
locales-pending/fix.ftl
Outdated
# $data_broker_count is the number of data brokers scanned monthly (As of adding this, it is 190). This number will always be plural. | ||
fix-flow-data-broker-profiles-automatic-remove-features-monthly-scan = Monthly scan of { $data_broker_count } data broker sites that may be selling your personal info |
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.
Same note as above: can we get rid of the number? If not, we need a plural form to trigger Pontoon UI, even if it's always > 1.
235bee5
to
1ae644a
Compare
1ae644a
to
3353502
Compare
Code review suggestions: Fluent string fixes Co-authored-by: Francesco Lodolo <[email protected]> Co-authored-by: Peter deHaan <[email protected]>
…entions, fix spacing, use brand variables
…s to use PascalCase
…line, H3/H4 typography sizes
084d7ba
to
0b6a8d9
Compare
Summary
This PR adds initial routes for the entire guided fix flow, and builds out the initial Data Broker scan results page. Additional pages (welcome, manual removal, automatic removal)
TODO
Add logic to next/prev buttonsSee known issuesAdd logic/state to ℹ️ button next to "View your info on these sites" textSee known issuesAdd (Number) on progress barSee known issuesKnown issues:
page.tsx
are currently server-sideOther Notes
Testing
⭐ Setup: You need scan results first: Please run through the welcome onboarding flow (
/redesign/user/welcome
) first.Testing steps:
/redesign/user/dashboard/
redesign/user/dashboard/fix/data-broker-profiles/start-free-scan
/redesign/user/dashboard/fix/data-broker-profiles/view-data-brokers
Screenshots
TBD