-
Notifications
You must be signed in to change notification settings - Fork 72
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
Support the “minimal” TCF experience API and dynamically fetch “full” API response after banner is shown #5230
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
57ddce1
to
a51633d
Compare
fides Run #9716
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5230/merge
|
Run status |
Passed #9716
|
Run duration | 00m 38s |
Commit |
ffb8d1d00a ℹ️: Merge 4cb5277ef6358adc95c5811a5a7119fcaa7e237f into edb36c77d0585185863d565ec7f6...
|
Committer | Jason Gill |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
3e6ed7b
to
c6087d5
Compare
c6087d5
to
d68376a
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.
Nice work so far @gilluminate - I've added some suggestions / questions but mostly looking for a test that checks:
- banner renders minimal response fields initially
- follow-up API call is made after banner renders for full experience
- banner updates to render full experience response
In addition, I'd like clarification around whether or not this is scoped to just TCF. It seems to me that this minimal API call is made across the board, not just for TCF experiences, right?
// the user's preferred language, otherwise use the "accept-language" header | ||
// provided by the browser. If all else fails, use the default. | ||
const fidesLocale = | ||
(req.query.fides_locale as string) || req.cookies?.fides_locale; |
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.
do we set fides_locale
anywhere across our ecosystem? Who uses this that we know of?
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 believe MV is using it on a few of their sites that are language specific. Is there a specific concern you are considering here?
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.
nope, just curious about the wider implications across the ecosystem ✔️
const fidesLocale = | ||
(req.query.fides_locale as string) || req.cookies?.fides_locale; | ||
const userLanguageString = | ||
fidesLocale || req.headers["accept-language"] || DEFAULT_LOCALE; |
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.
what is the default locale set to?
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.
default is "en"
@@ -1118,8 +1100,6 @@ button.fides-banner-button.fides-menu-item:not([aria-pressed="true"]):hover { | |||
} | |||
|
|||
div#fides-banner-inner-container { | |||
display: flex; | |||
flex-direction: column; |
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.
Can you explain why these CSS changes were needed or generally what these changes do?
We're also making some CSS changes here that we need to eval to see if there are any conflicts- clients/fides-js-extensions/nytimes/src/ext.css
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.
all CSS changes are being made as part of this layout update ticket https://ethyca.atlassian.net/browse/PROD-2611 which is pretty heavily tied to the other TCF updates so I'm including them in this PR. All of these affect TCF only, and they affect the banner only. No style updates for modal at all and no updates for non-TCF banner.
{!onVendorPageClick && !!vendorCount && ( | ||
<span className="fides-vendor-count">{vendorCount}</span> | ||
)} | ||
{onVendorPageClick && vendorCount && ( | ||
{onVendorPageClick && !!vendorCount && ( |
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 change ensures that 0 vendors causes this not to render
@@ -1118,8 +1100,6 @@ button.fides-banner-button.fides-menu-item:not([aria-pressed="true"]):hover { | |||
} | |||
|
|||
div#fides-banner-inner-container { | |||
display: flex; | |||
flex-direction: column; |
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.
all CSS changes are being made as part of this layout update ticket https://ethyca.atlassian.net/browse/PROD-2611 which is pretty heavily tied to the other TCF updates so I'm including them in this PR. All of these affect TCF only, and they affect the banner only. No style updates for modal at all and no updates for non-TCF banner.
// the user's preferred language, otherwise use the "accept-language" header | ||
// provided by the browser. If all else fails, use the default. | ||
const fidesLocale = | ||
(req.query.fides_locale as string) || req.cookies?.fides_locale; |
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 believe MV is using it on a few of their sites that are language specific. Is there a specific concern you are considering here?
const fidesLocale = | ||
(req.query.fides_locale as string) || req.cookies?.fides_locale; | ||
const userLanguageString = | ||
fidesLocale || req.headers["accept-language"] || DEFAULT_LOCALE; |
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.
default is "en"
@eastandwestwind Since we don't know what the experience will be when the initial call is made, so we do supply the minimal request to the api endpoint with the understanding that if TCF is being returned, we want the minimal version. It will be ignored otherwise. I'll add this as a comment in the code. |
0ed7a4d
to
cb7903e
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.
Awesome work on the tests! Just want to add a Changelog, and we're good to go here!
if (props.overrideExperience) { | ||
experience = props.overrideExperience(experience); | ||
if (props?.options?.tcfEnabled) { | ||
stubTCFExperience({ |
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.
nice work adding this stub to replicate minimal experience!
fix unit test
a04be3e
to
4cb5277
Compare
fides Run #9719
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #9719
|
Run duration | 00m 38s |
Commit |
4a2dc3a958: Support the “minimal” TCF experience API and dynamically fetch “full” API respon...
|
Committer | Jason Gill |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
Closes PROD-2608 and PROD-2611
Description Of Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md