Adding Fides.showModal #4617

galvana · 2024-02-15T19:55:48Z

Closes PROD-1689

Description Of Changes

Adds Fides.showModal to be able to manually trigger the modal if supported by the current experience configuration.

Steps to Confirm

Start the fides server
Navigate to the clients directory and run turbo run dev
Navigate to http://localhost:3001/fides-js-demo.html?geolocation=us-ca
Run window.Fides.showModal in the console, verify the console appears

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-02-15T19:55:53Z

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		Feb 16, 2024 9:31pm

galvana · 2024-02-15T20:03:03Z

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

@@ -100,7 +102,16 @@ const Overlay: FunctionComponent<Props> = ({
    return () => clearTimeout(delayBanner);
  }, [setBannerIsOpen]);

+  const showModal = () => {
+    if (!isModalLinkFound) {
+      document.body.classList.add("fides-modal-link-shown");


Only adds the fides-modal-link-shown class to the body if the modal link isn't found

nit: I think it's slightly safer to do a different classname to not redefine the semantic meaning of this class. In other words, I could see someone already doing this:

.fides-modal-link-shown { font-size: 40px }

...since today we only ever add that class to the individual link element, that would have been "safe". But if we start adding this class to the body that could be disastrous!

Ah! I misread the class name, updated to fides-overlay-modal-link-shown

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

cypress · 2024-02-15T20:18:44Z

Passing run #6296 ↗︎

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

Details:

Merge `ea025f8` into `57011ac`...
Project: fides	Commit: `0c57f7769d ℹ️`
Status: Passed	Duration: 00:34 💡
Started: Feb 16, 2024 9:42 PM	Ended: Feb 16, 2024 9:43 PM

Review all test suite changes for PR #4617 ↗︎

NevilleS

I know this is WIP, but it looks good to me! Some nits and suggestions

NevilleS · 2024-02-15T20:18:42Z

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

+    if (!isModalLinkFound) {
+      document.body.classList.add("fides-modal-link-shown");
+    }
+    setBannerIsOpen(false);


nit: can't this be moved into handleOpenModal()?

NevilleS · 2024-02-15T20:35:35Z

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

@@ -100,7 +102,16 @@ const Overlay: FunctionComponent<Props> = ({
    return () => clearTimeout(delayBanner);
  }, [setBannerIsOpen]);

+  const showModal = () => {
+    if (!isModalLinkFound) {
+      document.body.classList.add("fides-modal-link-shown");


nit: I think it's slightly safer to do a different classname to not redefine the semantic meaning of this class. In other words, I could see someone already doing this:

.fides-modal-link-shown { font-size: 40px }

...since today we only ever add that class to the individual link element, that would have been "safe". But if we start adding this class to the body that could be disastrous!

NevilleS · 2024-02-15T20:40:28Z

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

-          setBannerIsOpen(false);
-          handleOpenModal();
-        };
+        modalLink.onclick = window.Fides.showModal;


👍

This is pretty much what I was imagining, yup!

As a slight bonus to this... I think we should consider switching this to use addEventListener("click", (evt) => { ... }) instead of onclick = () => { ... }. This isn't the core ask of the ticket, but looking at this code again reminds me that, in general onclick feels fragile.

NevilleS

Couple more nits but this looks good!

NevilleS · 2024-02-16T00:04:58Z

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

@@ -110,19 +127,26 @@ const Overlay: FunctionComponent<Props> = ({
          options.debug,
          "Modal link element found, updating it to show and trigger modal on click."
        );
+        modalLinkRef.current = modalLinkEl;
        // Update modal link to trigger modal on click
        const modalLink = modalLinkEl;


You should delete this var and use the ref instead 👍

NevilleS · 2024-02-16T00:05:09Z

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

-          setBannerIsOpen(false);
-          handleOpenModal();
-        };
+        modalLink.addEventListener("click", window.Fides.showModal);


Suggested change

modalLink.addEventListener("click", window.Fides.showModal);

modalLinkRef.current.addEventListener("click", window.Fides.showModal);

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

NevilleS · 2024-02-16T00:07:44Z

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

@@ -100,7 +109,15 @@ const Overlay: FunctionComponent<Props> = ({
    return () => clearTimeout(delayBanner);
  }, [setBannerIsOpen]);

+  const showModal = () => {
+    if (!modalLinkRef.current) {
+      document.body.classList.add("fides-overlay-modal-link-shown");


Oh, I think I was confusing everyone about this.

I'd like us to add this class to the body immediately when the overlay renders, so that it's there and available as a utility class for the engineer to pull from.

So I'd do this right on line 120 instead, see next comment...

NevilleS · 2024-02-16T00:09:05Z

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

  useEffect(() => {
+    window.Fides.showModal = showModal;


Suggested change

window.Fides.showModal = showModal;

window.Fides.showModal = showModal;

document.body.classList.add("fides-overlay-modal-link-shown")

Basically that CSS class should be synced to whether or not the modal link should be shown and usable.

galvana · 2024-02-16T19:02:41Z

clients/privacy-center/cypress/e2e/show-modal.cy.ts

+
+    it("Should allow showModal", () => {
+      // eslint-disable-next-line cypress/no-unnecessary-waiting
+      cy.wait(100);


Give enough time for the Overlay.tsx component to run the useEffect that sets Fides.showModal

updates made

Adding Fides.showModal

c9ba17a

galvana requested a review from NevilleS February 15, 2024 20:02

galvana commented Feb 15, 2024

View reviewed changes

Fixing import

4ee3533

NevilleS requested changes Feb 15, 2024

View reviewed changes

galvana added 2 commits February 15, 2024 13:27

Updates based on PR feedback

e2fda66

Using ref to avoid duplicate element lookup

bc525cd

galvana requested a review from NevilleS February 15, 2024 21:41

NevilleS previously requested changes Feb 16, 2024

View reviewed changes

galvana added 2 commits February 16, 2024 09:02

Cypress tests

14e15d1

Fixing test

ff8d852

galvana requested a review from NevilleS February 16, 2024 18:58

galvana marked this pull request as ready for review February 16, 2024 18:58

galvana commented Feb 16, 2024

View reviewed changes

Kelsey-Ethyca approved these changes Feb 16, 2024

View reviewed changes

Updating change log

ea025f8

Kelsey-Ethyca merged commit 68c204a into main Feb 16, 2024
13 checks passed

Kelsey-Ethyca deleted the PROD-1689-fides-show-modal branch February 16, 2024 22:20

Kelsey-Ethyca pushed a commit that referenced this pull request Feb 16, 2024

Adding Fides.showModal (#4617)

cf63fba

cypress bot mentioned this pull request Feb 16, 2024

Release 2.30.0 #4613

Closed

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Fides.showModal #4617

Adding Fides.showModal #4617

galvana commented Feb 15, 2024 •

edited by jira bot

Loading

vercel bot commented Feb 15, 2024 •

edited

Loading

galvana Feb 15, 2024

NevilleS Feb 15, 2024

galvana Feb 15, 2024

cypress bot commented Feb 15, 2024 •

edited

Loading

NevilleS left a comment

NevilleS Feb 15, 2024

NevilleS Feb 15, 2024

NevilleS Feb 15, 2024

NevilleS left a comment

NevilleS Feb 16, 2024

NevilleS Feb 16, 2024

NevilleS Feb 16, 2024

NevilleS Feb 16, 2024

galvana Feb 16, 2024

	modalLink.addEventListener("click", window.Fides.showModal);
	modalLinkRef.current.addEventListener("click", window.Fides.showModal);

	window.Fides.showModal = showModal;
	window.Fides.showModal = showModal;
	document.body.classList.add("fides-overlay-modal-link-shown")

Adding Fides.showModal #4617

Adding Fides.showModal #4617

Conversation

galvana commented Feb 15, 2024 • edited by jira bot Loading

Description Of Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Feb 15, 2024 • edited Loading

Passing run #6296 ↗︎

Review all test suite changes for PR #4617 ↗︎

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

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

galvana commented Feb 15, 2024 •

edited by jira bot

Loading

vercel bot commented Feb 15, 2024 •

edited

Loading

cypress bot commented Feb 15, 2024 •

edited

Loading