-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add infrastructure for consent banner and link #3191
Conversation
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.
Looking hawt! Couple comments on this WIP
Passing run #1946 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
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.
Couple comments that I was thinking about recently - I think we can architect this in a super clean and functional way to make this easy to incrementally build, but want your $0.02 on it too
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.
Couple comments that I was thinking about recently - I think we can architect this in a super clean and functional way to make this easy to incrementally build, but want your $0.02 on it too
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.
Couple comments that I was thinking about recently - I think we can architect this in a super clean and functional way to make this easy to incrementally build, but want your $0.02 on it too
147e5dd
to
06e46cb
Compare
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.
OK, looking good so far!
There are some large structural asks in this review- hopefully we're at a point now where we have a solid test harness for all the logic so we can retool this to fit our liking.
Let me know what you think is realistic to achieve for a first version, and what we should split out and ticket as follow-up changes
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.
this is a really solid foundation, @eastandwestwind ! I'm going to keep playing with things today, but here are some comments for now. looking forward to building off of this!
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.
Couple more nits and a bug or two but this is super close!
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.
Couple more nits and a bug or two but this is super close!
… add geolocation constructor to be injested by fides API
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.
OK, ship it!
I still have a bunch of nitpicks but I pushed a few tidy up commits. Let's see if CI still passes!
Closes #3043 and #3051
Code Changes
'fides-consent-link'
somewhere on the page. We should either surface this info within the experience / notice flow OR make this configurable in privacy experiencesSteps to Confirm
Pre-requisites to test:
npm install turbo --global
clients
folder, runnpm install
.To test:
clients/privacy-center
folder, runturbo run dev
, then navigate to http://localhost:3000/fides-js-components-demo.htmlfidesConfig
object passed intoFides.init()
inprivacy-center/public/fides-js-components-demo.html
Pre-Merge Checklist
CHANGELOG.md
Follow-up Tickets
consent-banner.cy.ts
tofides-js
instead ofprivacy-center
- Moveconsent-banner.cy.ts
e2e test tofides-js
instead ofprivacy-center
#3274fides-js
components into a specified HTML container #3293