-
Notifications
You must be signed in to change notification settings - Fork 77
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
Paging for TCF overlay #4463
Paging for TCF overlay #4463
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #5442 ↗︎Details:
Review all test suite changes for PR #4463 ↗︎ |
@@ -129,6 +118,8 @@ const TcfPurposes = ({ | |||
onChange({ newEnabledIds, modelType: activeData.purposeModelType }) | |||
} | |||
renderToggleChild={(purpose) => <PurposeDetails purpose={purpose} />} | |||
// This key forces a rerender when legal basis changes, which allows paging to reset properly |
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 neat pattern, more details here: https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-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.
oh very cool!
const declarations: GvlDataDeclarations | undefined = | ||
// @ts-ignore this type doesn't exist in v2.2 but does in v3 |
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.
unrelated to this PR, but moved the ts-ignore down since it's this line that needs it
@@ -226,34 +281,18 @@ const TcfVendors = ({ | |||
const [activeLegalBasisOption, setActiveLegalBasisOption] = useState( | |||
LEGAL_BASIS_OPTIONS[0] | |||
); | |||
const activeData: { |
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 mostly moving the contents of the vendor data to a child component. This allows us to think of the toggles ("consent" vs "legitimate interest") as top level filters, which inform what data gets shown in the child component. We need either the legal basis filter or paging to be top level (we page through by legal basis)
.map((_, i) => `ctl_${i}`); | ||
const VENDOR_IDS = [...GVL_IDS, ...SYSTEM_IDS]; | ||
|
||
beforeEach(() => { |
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 beforeEach
is large in order to get us set up with a lot of vendors in all the right places. but it means the tests that follow are more readable!
UAT tested and looking really good functionally. went thru all the AC from the jira ticket and ensured things looked good locally after adding all 1k+ vendors from compass. screenshot below shows the paging on the "vendors" tab, but other tabs were also checked. everything was snappy on the FE (the BE did take a while to provide the overlay contents, but that's expected and will be mitigated by caching and other future performance improvements) 👍 update: the one piece of this that's still not working is saving consent preferences in this state of 1k+ systems/vendors. That has nothing to do with the paging functionality or with the changes in this PR specifically. it's a BE issue that is captured in https://ethyca.atlassian.net/browse/PROD-1472. it was just surfaced when UAT testing this feature. ![]() |
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.
Great work on this @allisonking ! I like how you've component-ized the PagingButtons so that we can use this in other places in the future too 🚀
|
||
// Add lots of vendors so that we can page | ||
VENDOR_IDS.forEach((id, idx) => { | ||
const { record, relationship, embedded } = mockTcfVendorObjects({ |
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.
ohhh this is awesome, thanks for adding this comprehensive testing!
* 2. `relationship`: a filled out TCFVendorRelationships object | ||
* 3. `embedded`: an EmbeddedVendor object which goes inside Purposes and Features | ||
*/ | ||
export const mockTcfVendorObjects = ( |
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.
🌟
Updated vendor label stacking (context): Screen.Recording.2023-12-01.at.2.57.22.PM.mov |
Closes https://ethyca.atlassian.net/browse/PROD-1369
Description Of Changes
https://www.loom.com/share/c568d4b4a975487195b1124eb23d4aec?sid=2512d35a-7672-406b-932d-7d2066257fe5
Code Changes
usePaging
for easy paging logic across componentschunkItems
Steps to Confirm
turbo dev
Pre-Merge Checklist
CHANGELOG.md