-
Notifications
You must be signed in to change notification settings - Fork 72
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
Update fides.js to support multiple descriptions (banner, overlay) and render HTML descriptions #4542
Update fides.js to support multiple descriptions (banner, overlay) and render HTML descriptions #4542
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Passing run #5827 ↗︎
Details:
Review all test suite changes for PR #4542 ↗︎ |
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.
Just some comments to help reviewers...
src/fides/api/alembic/migrations/versions/f396c1f84b0f_add_banner_description_to_experience_.py
Show resolved
Hide resolved
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.
BE looks solid to me! i think you've got all the fundamental pieces you need, i know there's at least one TODO so i'll hold off on approving. i like the HtmlStr
custom type and validator.
i'm still not the most familiar with the use cases, but just want to make sure - should we populate these fields at all in the default "out of the box" experience configs we define in src/fides/data/privacy_notices/privacy_experience_config_defaults.yml
?
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.
FE looks great! thanks for working on this!
No, I don't think so. In general, the defaults that set the banner & modal descriptions/titles to be identical are reasonable, so I felt like we should leave our defaults unchanged and have these new fields as |
In the UI, do we drop back to using the existing title/description if banner_title/banner_description don't exist? If not, then yes, we'll need to populate this template. Further, current behavior of EDIT: OK! looks like we're all good here then. We drop back to using title/description if banner title/description doesn't exist. No template updates needed. |
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.
looks good so far, backend is fairly straightforward since these new fields are allowed to be nullable - will look again when HtmlStr validation is in -
src/fides/api/alembic/migrations/versions/f396c1f84b0f_add_banner_description_to_experience_.py
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4542 +/- ##
==========================================
- Coverage 87.06% 87.06% -0.01%
==========================================
Files 334 334
Lines 20539 20554 +15
Branches 2650 2651 +1
==========================================
+ Hits 17883 17896 +13
- Misses 2185 2186 +1
- Partials 471 472 +1 ☔ View full report in Codecov by Sentry. |
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.
A few more comments for @allisonking
clients/admin-ui/src/types/api/models/ExperienceConfigCreate.ts
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/privacy-experience/form/OverlayForm.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/types/api/models/ExperienceConfigCreate.ts
Outdated
Show resolved
Hide resolved
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.
some minor nits on the validation that i wouldn't consider blockers, generally looks good and i think you've plugged the validation into the right places! i'm not an HTML sanitization expert but the library you're using looks like a good standard, and i trust your judgement there 👍
…ort-multiple-html-descriptions
OK, I think I've got everything covered here. Switching to some final polish & UAT. |
OK, @mfbrown and I did some UAT together on the Admin UI form. Here's the main before/after screenshots for that:
Lots of copy edits in there that @mfbrown and I did together. It's still a bit clumsy and wordy, but we felt that it was best to be "clear" before trying to get clever with wording and didn't have great ideas 😄 UAT complete ✅ |
…ort-multiple-html-descriptions
…d render HTML descriptions (#4542)
Closes PROD-1528
Closes PROD-1338
...and adds support for HTML descriptions (unticketed)
Description Of Changes
As more & more customers are using our consent overlay, we're needing some more flexible options for configuring the major text elements of the banner & modal. This PR adds two major features:
Code Changes
allowHTMLDescription
to optionally allow HTMLallowHTMLDescription
optionnh3
for HTML sanitization checksSteps to Confirm
FIDES_PRIVACY_CENTER__ALLOW_HTML_DESCRIPTION=true
, then configure banner & modal description fields with HTML and test their changes in the Cookie HousePre-Merge Checklist
CHANGELOG.md