Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TCF experience design improvements #4222
TCF experience design improvements #4222
Changes from 10 commits
c13775c
2057c1f
ad84405
0be3aea
ae8fb9c
1c5badd
ff59616
6830081
d33f8ce
baf9545
25211f2
32aa418
e689a4c
528788b
2803482
db39bfe
b208e11
0d66d64
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I didn't even know there was a
String
! looks like we should preferstring
tho: https://stackoverflow.com/questions/14727044/what-is-the-difference-between-types-string-and-stringThere 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.
or even better, would be to restrict this to the strings it can be, i.e.
"TCF"|"notices"
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 ended up going away after refactoring the GPC check
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.
Passing in
bannerType
to hide theGpcBadge
for the TCF banner/modalThere 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.
hmm if its only purpose is whether or not to show the gpc badge, maybe instead of
bannerType
we make the prophideGpc
, then change above. I think that makes it a bit clearer for someone using this component exactly what that prop is going to doThere 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.
actually maybe a better idea is to modify the function
getGlobalPrivacyControl
to querywindow.Fides.options.tcfEnabled
. if it's enabled, return false. then I don't think gpc would ever render for TCF, and it'd be tied to gpc logic, as opposed to component logic, which is nice!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.
I like that, much cleaner!
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.
same thing here—see my meandering comment thoughts about on
bannerType