-
Notifications
You must be signed in to change notification settings - Fork 206
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-3378 - Data broker manual removal #5287
base: main
Are you sure you want to change the base?
Conversation
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
| "removed" | ||
| "removal_under_maintenance"; |
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.
@rhelmer I believe this would be our first time consuming this endpoint for the manual data broker removal feature.
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
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 is looking good so far! There are some open comments and questions I’m having:
- Before landing any parts of this feature we’ll need to make sure to guard it behind a feature flag.
- Additionally, we would need to add telemetry (potentially in a follow-up) to the CTAs to fulfill the acceptance criteria.
- For smaller viewports it might be better to stack the buttons:
src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/dashboard/View.tsx
Show resolved
Hide resolved
<div className={styles.removalContentSection}> | ||
<dt> | ||
{l10n.getString( | ||
"data-broker-removal-maintenance-steps-to-remove-header", |
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 header would need to be:
"data-broker-removal-maintenance-steps-to-remove-header", | |
"data-broker-removal-maintenance-rationale-header", |
data={props.stepDeterminationData} | ||
hideProgressIndicator={detailedRemovalGuide} | ||
> | ||
{!detailedRemovalGuide ? dataBrokerInformation : removalGuideInstructions} |
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.
The only way to go back to the exposures when the “removal instructions” are open is by clicking “Back to exposures” (which for many viewport sizes is out of sight). I’m wondering if it would not be more intuitive to follow our existing pattern of showing explainers in dialogs. If that was already decided against by UX and/or is out of scope we should at least make sure that the closing “x” button does not link back to the dashboard.
@@ -85,6 +86,21 @@ describe("ScanResultCard", () => { | |||
expect(innerDescription).toBeInTheDocument(); | |||
}); | |||
|
|||
// Data broker removal under maintenance | |||
it("shows the right description for a scan result card where removal is under maintenance", () => { |
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: Should we also add a test that checks that the description is not shown for other statuses?
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
2 similar comments
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
Co-authored-by: Francesco Lodolo <[email protected]>
Preview URL 🚀 : https://blurts-server-pr-5287-mgjlpikfea-uk.a.run.app |
References:
Jira: MNTOR-3378
Figma: https://www.figma.com/design/DTbmXzCQCw2PxXpHQW8yfW/Monitor-MVP-Enhancements?node-id=3809-6501&t=EniBV7WqvPVnuivk-4
Path: /user/dashboard/fix/data-broker-profiles/removal-under-maintenance
Description
Screenshot (if applicable)
Not applicable.
How to test
Checklist (Definition of Done)