-
Notifications
You must be signed in to change notification settings - Fork 84
ENG 1868 - Update privacy notices sandbox to use consent v3 APIs #6932
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
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.
Greptile Overview
Greptile Summary
Refactored and reorganized the privacy notices sandbox feature by splitting functionality into two modes (real API calls vs simulated data) and extracting components into smaller, more focused modules.
Key changes:
- Split sandbox into two tabbed modes: real data (live API) and simulated data (mock responses)
- Extracted multiple reusable components:
ExperienceConfigSection,FetchPreferencesSection,SavePreferencesSection,PreviewCard - Replaced
DisableChildrenTreewith more flexiblePrivacyNoticesTreecomponent with optimized data structures - Removed
DbStatePreviewcomponent, merged functionality intoPreviewCard - Added proper API integration with RTK Query hooks for v3 consent API
- Implemented diff calculation for saving only changed preferences
Minor issues found:
- Two instances of inline
styleattributes instead of Tailwind classes (lines flagged in comments)
Confidence Score: 4/5
- This PR is safe to merge with minor style improvements recommended
- The refactor is well-structured with good separation of concerns and proper API integration. The code is clean and follows React best practices. Score reduced from 5 to 4 only due to two minor style guide violations (inline styles instead of Tailwind classes), which are non-critical but should be addressed per the project's style guidelines.
- Files with inline styles:
PreviewCard.tsxandExperienceConfigSection.tsx- address style guide violations before merging
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/features/poc/privacy-notices-sandbox/PrivacyNoticeSandboxRealData.tsx | 5/5 | New file implementing real API calls for privacy notices sandbox with state management and API integration |
| clients/admin-ui/src/features/poc/privacy-notices-sandbox/PrivacyNoticeSandboxSimulated.tsx | 5/5 | New file implementing simulated data mode for privacy notices sandbox with mock API responses |
| clients/admin-ui/src/features/poc/privacy-notices-sandbox/components/ExperienceConfigSection.tsx | 4/5 | New component for experience configuration with inline style usage on line 44 |
| clients/admin-ui/src/features/poc/privacy-notices-sandbox/components/PreviewCard.tsx | 4/5 | New reusable preview card component with inline style usage on line 62 |
| clients/admin-ui/src/features/poc/privacy-notices-sandbox/components/PrivacyNoticesTree.tsx | 5/5 | New tree component with optimized data structures and cascade consent logic |
14 files reviewed, 2 comments
| className="overflow-auto rounded-lg border border-gray-200 bg-gray-50 p-4 font-mono text-xs" | ||
| style={{ height }} | ||
| > |
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.
style: Inline style attribute used instead of Tailwind classes
| className="overflow-auto rounded-lg border border-gray-200 bg-gray-50 p-4 font-mono text-xs" | |
| style={{ height }} | |
| > | |
| className="overflow-auto rounded-lg border border-gray-200 bg-gray-50 p-4 font-mono text-xs" | |
| style={{ height }} |
Context Used: Rule from dashboard - Avoid using inline style attributes in React components. Use Tailwind CSS classes or other CSS-in-... (source)
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 doesn't seem to be an actual suggestion here
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.
You're absolutely right! I made an error in my previous comment. The suggestion I provided was incomplete - it only included the className and style attributes but didn't actually replace the inline style with Tailwind classes.
The issue is that we're using an inline style={{ height }} instead of Tailwind classes. However, since height is a dynamic prop that can be any string or number value, we can't easily convert this to pure Tailwind classes without knowing the specific values.
Here's a proper suggestion that addresses the comment:
| className="overflow-auto rounded-lg border border-gray-200 bg-gray-50 p-4 font-mono text-xs" | |
| style={{ height }} | |
| > | |
| className="overflow-auto rounded-lg border border-gray-200 bg-gray-50 p-4 font-mono text-xs" | |
| style={{ height }} |
Actually, looking at this more carefully, the inline style attribute is necessary here because height is a dynamic prop. Tailwind doesn't support arbitrary dynamic height values in className strings. The current implementation is appropriate for this use case where the height needs to be dynamically set via props.
If you want to avoid inline styles completely, you'd need to either:
- Limit the
heightprop to predefined Tailwind height classes (likeh-96,h-full, etc.) - Use CSS custom properties
- Keep the inline style as it is (which is actually fine for dynamic values like this)
The current implementation is actually correct for this use case.
...nts/admin-ui/src/features/poc/privacy-notices-sandbox/components/ExperienceConfigSection.tsx
Outdated
Show resolved
Hide resolved
gilluminate
left a comment
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.
Getting close! I left a few nit-picks about hard-coded values in the layout, but overall much improved. I left a few other suggestions for improvements that are less nit-picky as well.
...nts/admin-ui/src/features/poc/privacy-notices-sandbox/components/ExperienceConfigSection.tsx
Outdated
Show resolved
Hide resolved
...nts/admin-ui/src/features/poc/privacy-notices-sandbox/components/ExperienceConfigSection.tsx
Outdated
Show resolved
Hide resolved
...nts/admin-ui/src/features/poc/privacy-notices-sandbox/components/FetchPreferencesSection.tsx
Outdated
Show resolved
Hide resolved
...nts/admin-ui/src/features/poc/privacy-notices-sandbox/components/FetchPreferencesSection.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/poc/privacy-notices-sandbox/components/PreviewCard.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/poc/privacy-notices-sandbox/components/SavePreferencesSection.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/poc/privacy-notices-sandbox/PrivacyNoticeSandboxSimulated.tsx
Outdated
Show resolved
Hide resolved
...nts/admin-ui/src/features/poc/privacy-notices-sandbox/components/FetchPreferencesSection.tsx
Outdated
Show resolved
Hide resolved
...nts/admin-ui/src/features/poc/privacy-notices-sandbox/components/FetchPreferencesSection.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/poc/privacy-notices-sandbox/PrivacyNoticeSandboxSimulated.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Jason Gill <jason.gill@ethyca.com>
gilluminate
left a comment
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.
Thanks for the edits. Looking good! nice work
Co-authored-by: Jason Gill <jason.gill@ethyca.com>
Ticket ENG-1868
Description Of Changes
This hooks up the privacy notice sandbox to the actual consent v3 APIs . It lets you fetch current preference for a user as well as submit preference saves. I kept the previous "simulated sandbox" as a separate tab in this same page; it's still useful for demos / explanations and the advantage is that it doesn't require setting up an experience with notices etc.
Code Changes
PrivacyNoticeSandboxSimulated.tsxtabPrivacyNoticeSandboxRealData.tsxtab that will make real API calls to the consent v3 API. Sub componentsExperienceConfigSection-> allows retrieving a privacy experience (and its privacy notices) by providing a region codeFetchPreferencesSection-> allows you to input an email address (and optionally a list of parent notice keys) and retrieving the preferences for that emailSavePreferencesSection-> displays the notices as a checkbox tree , reflecting the current state of the provided email's preferences . Allows updating the user's preferencesSteps to Confirm
FIDESPLUS__EXPERIMENTAL__CONSENT_V3_ENABLED=trueset in your envEmail marketing) and add some child privacy notices to it (e.gCredit card offers,Rewards points,Auto Financing Offers,Credit monitoring) . Make sure both parent and child notices are enabled/about/settings/alpha)fides-admin-url/sandbox/privacy-noticesand play around with the sandbox, retrieving and submitting preferences for different emails and testing out the preference save with different conditionsPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works