Include content policy state in RRM setup success notification GA events.#12126
Conversation
|
Storybook is ready:
|
|
Build files for 835f284 have been deleted. |
nfmohit
left a comment
There was a problem hiding this comment.
This looks great, thanks @techanvil!
I've just left one comment for your consideration.
| ); | ||
| } ); | ||
|
|
||
| describe.each( [ |
There was a problem hiding this comment.
Shouldn't these tests be nested under GA event tracking? Also, I think we may be able to combine both test cases into one. WDYT?
There was a problem hiding this comment.
Thanks Nahid - they should be under GA event tracking. Re. combining the tests - I did start off taking that approach, but the amount of conditionality in the tests made me decide to separate them.
However, I've taken another look and refactored them to combine, and I think it's OK. Please see what you think.
nfmohit
left a comment
There was a problem hiding this comment.
LGTM, thank you @techanvil!
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist