-
Notifications
You must be signed in to change notification settings - Fork 290
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
Fix Analytics VRT. #9516
Fix Analytics VRT. #9516
Conversation
Build files for 0538f6e are ready:
|
Size Change: 0 B Total Size: 1.82 MB ℹ️ View Unchanged
|
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.
Good to get the AudienceSegmentationSetupSuccessSubtleNotification
fix in here but I'd suggest backing out the GoogleTagIDMismatchNotification
one as discussed in the comment for it, please see what you think.
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.
Hmm, although it's commendable in principle to fix this here, the GoogleTagIDMismatchNotification
VRT already has a fix in the open PR #9469, as mentioned previously on Slack. It looks like that fix avoids the need to recreate the reference images for it.
As it's out of scope for this issue, I'd suggest backing out the change to avoid creating a conflict for the other PR and updating the reference images unnecessarily...
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.
LGTM, thanks @tofumatt!
Summary
Addresses issue:
Also fixed the other Google Tag VRT that has been failing.
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