Skip to content
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

Notice toggle table for fides-js #3291

Merged
merged 34 commits into from
May 19, 2023
Merged

Notice toggle table for fides-js #3291

merged 34 commits into from
May 19, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented May 11, 2023

Most of #3161

Still need to:

Code Changes

  • Update rollup to support React (kind of, doesn't work 100% yet 🥲 )
  • Add a new component ConsentModal.tsx
    • ❗ doesn't follow a11y rules yet. I think it may be worth using a 3rd party for this one since modals are tricky, but I haven't gotten React importing to work quite right yet
  • New component NoticeToggle which renders the bulk of the modal contents
  • New Toggle component which adheres to a11y rules
  • Accordion behavior for the notices which also adheres to a11y rules
  • Some progress towards using our new CSS vars (which is actually part of Refactor consent components to use CSS variables that match the brandable colour palette from designs #3258)
  • Cypress tests (we should be able to add more once the endpoints are linked up)

Steps to Confirm

  • Run npm run dev-pc in the clients folder
  • Navigate to http://localhost:3000/fides-js-components-demo.html
  • In the banner that comes up, click "Manage preferences" which will open the modal
  • Toggle the modal and click "Save". Open the console log to see the checked/unchecked values

Pre-Merge Checklist

Description Of Changes

image

@cypress
Copy link

cypress bot commented May 11, 2023

Passing run #2069 ↗︎

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

Details:

Merge 4174c97 into 9a26ca1...
Project: fides Commit: 5840fcd219 ℹ️
Status: Passed Duration: 00:50 💡
Started: May 19, 2023 6:08 PM Ended: May 19, 2023 6:09 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@allisonking
Copy link
Contributor Author

I've been really struggling getting NoticeToggleTable.tsx to be a React component that the Preact app can import 🥲

What I've tried:

  • To denote that NoticeToggleTable.tsx is React, I set a "pragma" of /** @jsx createElement */ at the top of the file, then import { createElement } from "react". My vscode is good with this, and I made a few tweaks to tsconfig.json too (though I don't know if these were strictly needed)
  • When I try to build fides-js, I get
copied:
  dist/fides.css → ../privacy-center/public/lib/fides.css
  dist/fides.js → ../privacy-center/public/lib/fides.js
created dist/fides.js in 408ms

src/fides.ts → dist/fides.mjs...
[!] RollupError: "createElement" is not exported by "../node_modules/react/index.js", imported by "src/components/NoticeToggleTable.tsx".
https://rollupjs.org/troubleshooting/#error-name-is-not-exported-by-module
src/components/NoticeToggleTable.tsx (3:9)
1: /** @jsx createElement */
2: 
3: import { createElement } from "react";
            ^
  • I've added the preact rollup aliases but that didn't seem to make a difference
  • I also played around with commonjs configurations but couldn't get one that made a difference
  • I'm wondering if I need to use a babel plugin, but I don't think I understand how rollup works well enough. I tried for a little bit with little difference to the error
  • I've also tried import React from 'react' and using /** @jsx React **/ but got a similar error

I'm going to try to tackle other parts of this ticket for now. I don't think it'd be the worst thing if the apps don't share this component, since Privacy Center can build its own version of this component with chakra which would be much easier. But it'd be nice to get this figured out at some point!

@eastandwestwind
Copy link
Contributor

ah, that's unfortunate to hear. Have you tried adding the rollup typescript plugin?- https://www.npmjs.com/package/rollup-plugin-typescript2

I'm reading https://stackoverflow.com/a/70163537 which seems to be a similar issue, using Rollup.

If this takes too much time, though, I'm OK with not sharing the toggle component between fides-js and privacy center for now. This can be a follow-up item.

Base automatically changed from banner-infra to main May 13, 2023 19:20
@allisonking
Copy link
Contributor Author

turns out I just needed to alias in two places! 🤦‍♀️ 0ddc5c4 thanks for the assist @eastandwestwind 🙌

@allisonking
Copy link
Contributor Author

status so far:
image

and the toggles can update/change their data accordingly.

I'm having trouble importing the NoticeToggleTable into the privacy-center, and I'm thinking there's more rollup investigation needed there. I'll probably punt on that for now, and continue trying to get fides-js mostly working. What's left:

  • divider lines
  • accordion to show more info about the notice
  • close the modal on save
  • mock out the API calls we need with sample payloads. this will be very helpful for testing!
  • Cypress tests
  • experiment with using https://github.com/KittyGiraudel/react-a11y-dialog to take care of our modal a11y needs

@allisonking allisonking changed the title Notice toggle table for fides-js and privacy center Notice toggle table for fides-js May 16, 2023
Copy link
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested manually and it's looking good so far! Nice to see you even got the background scrolling working behind the modal :)

https://www.loom.com/share/a9038b903dbd4177b346a82a861570a5

On thing I want to point out is I see that the modal does not currently:

  1. close the banner on save / accept all / reject all- could we add this in easily? Since testing data is passed in on every page load, the banner should still re-open on subsequent page loads, so testing won't be inhibited by this.
  2. write the cookie on save, like we do for accept all / reject all- do we have a follow-up ticket for this, or should I handle it on my end?

Update rollup to support React (kind of, doesn't work 100% yet 🥲 )

Could you describe how to replicate the part that doesn't work with React?

clients/fides-js/src/components/ConsentModal.tsx Outdated Show resolved Hide resolved
clients/fides-js/src/components/ConsentModal.tsx Outdated Show resolved Hide resolved
clients/fides-js/src/components/Overlay.tsx Outdated Show resolved Hide resolved
clients/fides-js/src/components/ConsentBanner.tsx Outdated Show resolved Hide resolved
@allisonking
Copy link
Contributor Author

  1. close the banner on save / accept all / reject all- could we add this in easily? Since testing data is passed in on every page load, the banner should still re-open on subsequent page loads, so testing won't be inhibited by this.
  2. write the cookie on save, like we do for accept all / reject all- do we have a follow-up ticket for this, or should I handle it on my end?

Update rollup to support React (kind of, doesn't work 100% yet 🥲 )

Could you describe how to replicate the part that doesn't work with React?

Thanks @eastandwestwind , all great points!

  1. I just happened to chat with Michael about this behavior, and it turns out we actually want to close the banner when the modal opens. The modal can't be exited until one of the save/accept/reject buttons is clicked. So I'll update this PR to do that
  2. Yes, I forgot about this part 😅 will do that!
  3. For React, I didn't commit the parts that weren't working, but basically if we try to add a React package, then import it and use it, the build will fail. I was trying with react-a11y-dialog

@eastandwestwind
Copy link
Contributor

Testing functionality now and all is looking good! The banner closes when the modal is opened, and I see the modal preserves state of consented notices when I reload the page to re-trigger the banner. Also the cookie is appropriately updated, along with the window.Fides obj upon saving consent preferences.

I want to confirm- if we have legacy consent preferences, we currently don't override them, just ensure that they remain in the cookie. The only use case where we touch legacy consent preferences is if the overlay is disabled, and we must use read from the legacy config.json in the Privacy Center. Does that align with your understanding as well?

@allisonking
Copy link
Contributor Author

I want to confirm- if we have legacy consent preferences, we currently don't override them, just ensure that they remain in the cookie. The only use case where we touch legacy consent preferences is if the overlay is disabled, and we must use read from the legacy config.json in the Privacy Center. Does that align with your understanding as well?

Oh, no I don't think I have that behavior. Am I understanding correctly that let's say...

  1. A user has a cookie with a consent entry of i.e. { data_sales: true }
  2. This new code now wants to build the cookies based on privacy notices. for now, we are using privacy notice IDs but we will move to keys based on names soon. but let's say it's still id for now, so we add { pri_123: true }
  3. The cookie should have { data_sales: true, pri_123: true} ?

That is not true right now, but I can probably merge the cookie values instead of overwriting them?

@eastandwestwind
Copy link
Contributor

Ok I think we should keep the functionality already in place, where we overwrite the cookie with the most recent notices. 2 reasons behind this:

  1. When we migrate customers to the new privacy experiences, we'll map the old keys to new notices entirely. So we won't have to worry about supporting legacy keys in this scenario.

  2. Separately, there's a case where a customer updates their notices so that the browser cookie contains the older invalid cookie key. In this scenario, we should try to honor the new notices as much as possible.

@allisonking allisonking merged commit 529da21 into main May 19, 2023
@allisonking allisonking deleted the aking/3162/pc-experience branch May 19, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants