Update FidesJS `fides_embed` option to support embedding both banner & modal components #4782

guncha · 2024-04-10T03:13:26Z

Description Of Changes

This turned out to be a bit of an adventure. Initially I wanted to allow embedding the banner and the modal in separate containers and after spending some time going in this direction, it turned out to be not possible without a lot of effort refactoring the code which I didn't feel comfortable doing (also time). The issue is that putting the banner and the modal in two different containers would require separating the components "all the way to the top", so to speak, which would mean they'd have entirely separate state and everything.

The approach here is to embed both the banner and the modal in the same fides-embed-container. The bannerIsOpen state in Overlay is used to control if the banner or the modal is shown.

Code Changes

Overlay component state management
CSS overrides when the banner is embedded

Steps to Confirm

The changes can be tested by using the fides_embed=true override. The banner will be shown first and it should be replaced with the Layer 2 "modal" when the "Manage preferences" link is clicked. Using fides_disable_banner=true will skip the banner and show the modal immediately, which is the previous behavior.

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

vercel · 2024-04-10T03:13:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Apr 11, 2024 1:43am

clients/fides-js/src/components/Overlay.tsx

cypress · 2024-04-10T03:25:22Z

Passing run #7172 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `479345e` into `a0faefa`...
Project: fides	Commit: `4382c4ae48 ℹ️`
Status: Passed	Duration: 00:37 💡
Started: Apr 11, 2024 1:54 AM	Ended: Apr 11, 2024 1:55 AM

Review all test suite changes for PR #4782 ↗︎

eastandwestwind

Tested this and looking great! It's almost there, just wanna fix that duplicate event firing.

banner:

modal:

Also confirmed that Admin UI preview mode still looks good:

clients/fides-js/src/components/fides.css

clients/fides-js/src/components/Overlay.tsx

clients/fides-js/src/components/ConsentModal.tsx

NevilleS

one quick nit! haven't reviewed in detail yet 👯

clients/fides-js/src/components/ConsentBanner.tsx

NevilleS

I'm comfy merging this as-is (once we're up to date with main & CHANGELOG) but I do have one more stretch ask...

Can you also update the developer docs here:

fides/clients/fides-js/src/docs/fides-options.ts

Line 53 in 262d2f3

* When `true`, require FidesJS to "embed" it's UI into a specific `<div>` on

(/clients/fides-js/src/docs/fides-options.ts)

A quick explanation there will be helpful, and you can reference the fides_disable_banner option to explain to a developer how to only embed the modal.

NevilleS

docs nit pick :)

NevilleS · 2024-04-10T19:33:22Z

clients/fides-js/docs/interfaces/FidesOptions.md

+Before version 2.34 only the consent modal was embedded while the banner was
+not shown at all, but since that version both will be embedded. To get the
+previous behavior also set `fides_disable_banner` to `true`.


nit: I'd rather not reference "previous versions" here, just state the fact:

will embed banner & modal

if you don't want banner, disable it

does that make sense?

… false

NevilleS

I think I fixed this, but want to get @guncha to double-check me

NevilleS · 2024-04-10T20:47:03Z

clients/privacy-center/cypress/e2e/consent-banner-tcf.cy.ts

-            fidesDisableBanner: true,
-          },
-          experience: experience.items[0],
+  describe("when fides_embed is true", () => {


This diff is hard to read, but I reorganized the specs to be:

when fides_embed is true

when fides_disable_banner is false

(NEW) test the layer 1 banner is shown

when fides_disable_banner is true

(all existing tests go here)

NevilleS · 2024-04-10T20:47:43Z

clients/fides-js/src/components/Overlay.tsx

@@ -76,7 +76,7 @@ const Overlay: FunctionComponent<Props> = ({
  const delayBannerMilliseconds = 100;
  const delayModalLinkMilliseconds = 200;
  const hasMounted = useHasMounted();
-  const [bannerIsOpen, setBannerIsOpen] = useState(options.fidesEmbed);
+  const [bannerIsOpen, setBannerIsOpen] = useState(false);


@guncha: I switched this back to default to false and it fixes things - but want to understand why before just shipping this... does this make sense?

The reason I did it was because otherwise, when fides_disable_banner is false, it would flash the modal and then re-render immediately and show the banner. My change didn't take into account fides_disable_banner so it should have had the opposite problem when it was set to true. Seems like it should be options.fidesEmbed && !options.fidesDisableBanner. Not quite sure why it would break the tests though so I'll do some investigation.

So yeah, this is the issue with just setting to to false:

Okay, I think I understand it now. There's this bit of code in Overlay.tsx:

fides/clients/fides-js/src/components/Overlay.tsx

Lines 144 to 150 in df46bf6

useEffect(() => {

const delayBanner = setTimeout(() => {

if (showBanner) {

setBannerIsOpen(true);

}

}, delayBannerMilliseconds);

return () => clearTimeout(delayBanner);

This is needed to first render ConsentBanner in the DOM with bannerIsOpen=false and then flip it to true after a short delay which will result in the removal of the fides-banner-hidden class and a nice entry animation.

In the embed case, we are not animating anything so we can jump to the final state, but it has to be the correct one, otherwise there will be a blip of the other component OR nothing being rendered. Still not sure how this worked before, but I'm glad the e2e test caught it.

I think it worked before because we were not rendering the banner at all with the embed mode. But this is a good catch, I've not used the performance tab in this way before, so I learned something new!

Ah, gotcha. Alright, I think overall this still feels like a fragile bit of state to manage (ie if we were being extra careful, that "banner is open" state feels like it's the job of the ConsentBanner component itself to handle it's own animation on mount) but this looks good now.

…& modal components (#4782) Co-authored-by: Neville Samuell <[email protected]>

Embed the banner

37b3186

guncha requested a review from NevilleS April 10, 2024 03:13

guncha commented Apr 10, 2024

View reviewed changes

clients/fides-js/src/components/Overlay.tsx Outdated Show resolved Hide resolved

eastandwestwind requested changes Apr 10, 2024

View reviewed changes

NevilleS requested changes Apr 10, 2024

View reviewed changes

clients/fides-js/src/components/ConsentBanner.tsx Show resolved Hide resolved

guncha added 2 commits April 10, 2024 14:08

Don't trigger the onOpen event twice

fdfc111

Formatting / lints

d071099

NevilleS changed the title ~~Embed the banner~~ Update FidesJS fides_embed option to support embedding both banner & modal components Apr 10, 2024

guncha added 2 commits April 10, 2024 14:50

Merge remote-tracking branch 'origin/main' into ga-embed-banner

4abdb30

Update changelog

0ea1cd8

NevilleS approved these changes Apr 10, 2024

View reviewed changes

Update Fides override docs

af4ac30

NevilleS requested changes Apr 10, 2024

View reviewed changes

guncha and others added 3 commits April 10, 2024 15:38

Update the docs

74c1579

Update TCF fides_embed tests to test with fides_disable_banner true &…

60e09e3

… false

Fix default state for bannerIsOpen

df46bf6

NevilleS requested changes Apr 10, 2024

View reviewed changes

guncha added 2 commits April 10, 2024 21:22

Update the bannerIsOpen logic

034bc94

Formatting

479345e

NevilleS approved these changes Apr 11, 2024

View reviewed changes

eastandwestwind approved these changes Apr 11, 2024

View reviewed changes

NevilleS merged commit 19b99df into main Apr 11, 2024
13 checks passed

NevilleS deleted the ga-embed-banner branch April 11, 2024 11:36

NevilleS added a commit that referenced this pull request Apr 11, 2024

Update FidesJS fides_embed option to support embedding both banner …

2d691e4

…& modal components (#4782) Co-authored-by: Neville Samuell <[email protected]>

NevilleS mentioned this pull request Apr 11, 2024

Add a few CSS classes to help with styling FidesJS button groups #4789

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update FidesJS `fides_embed` option to support embedding both banner & modal components #4782

Update FidesJS `fides_embed` option to support embedding both banner & modal components #4782

guncha commented Apr 10, 2024 •

edited

Loading

vercel bot commented Apr 10, 2024 •

edited

Loading

cypress bot commented Apr 10, 2024 •

edited

Loading

eastandwestwind left a comment

NevilleS left a comment

NevilleS left a comment

NevilleS left a comment

NevilleS Apr 10, 2024

NevilleS left a comment

NevilleS Apr 10, 2024

NevilleS Apr 10, 2024

guncha Apr 11, 2024

guncha Apr 11, 2024

guncha Apr 11, 2024

eastandwestwind Apr 11, 2024

NevilleS Apr 11, 2024

	useEffect(() => {
	const delayBanner = setTimeout(() => {
	if (showBanner) {
	setBannerIsOpen(true);
	}
	}, delayBannerMilliseconds);
	return () => clearTimeout(delayBanner);

Update FidesJS fides_embed option to support embedding both banner & modal components #4782

Update FidesJS fides_embed option to support embedding both banner & modal components #4782

Conversation

guncha commented Apr 10, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Apr 10, 2024 • edited Loading

cypress bot commented Apr 10, 2024 • edited Loading

Passing run #7172 ↗︎

Review all test suite changes for PR #4782 ↗︎

eastandwestwind left a comment

Choose a reason for hiding this comment

NevilleS left a comment

Choose a reason for hiding this comment

NevilleS left a comment

Choose a reason for hiding this comment

NevilleS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NevilleS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update FidesJS `fides_embed` option to support embedding both banner & modal components #4782

Update FidesJS `fides_embed` option to support embedding both banner & modal components #4782

guncha commented Apr 10, 2024 •

edited

Loading

vercel bot commented Apr 10, 2024 •

edited

Loading

cypress bot commented Apr 10, 2024 •

edited

Loading