-
Notifications
You must be signed in to change notification settings - Fork 291
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
Show 'Display visitor groups in dashboard' toggle even if GA is disco… #9349
Conversation
Build files for 286bb58 have been deleted. |
Size Change: +2.3 kB (+0.13%) Total Size: 1.8 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.
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, @ankitrox! LGTM 👍
Just a heads-up, I made a minor adjustment to rearrange the import and updated the QAB to ensure that Audience Segmentation is enabled to configure the audiences. Without this, the configured audiences would return as null
.
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.
@ankitrox, after reviewing the E2E test failures, it appears that some are related to the changes made in this PR. Could you please investigate and address them?
…ogle/site-kit-wp into enhancement/9264-visitor-group-toggle.
Thank you @hussain-t I've addressed the issues in E2E and there are no relevant errors. I can see that JS tests, VRT are failing in pipeline, but getting passed in local, so moving this to CR. |
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, @ankitrox. LGTM 👍
const configuredAudiences = useSelect( ( select ) => | ||
select( CORE_USER ).getConfiguredAudiences() |
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.
Hi @ankitrox, this change has introduced a regression as reported by @wpdarren in Slack.
We need to guard against the request to the audience-settings
endpoint with a check for audienceSegmentationEnabled
here:
const configuredAudiences = useSelect( ( select ) => | |
select( CORE_USER ).getConfiguredAudiences() | |
const configuredAudiences = useSelect( | |
( select ) => | |
audienceSegmentationEnabled && | |
select( CORE_USER ).getConfiguredAudiences() |
Please can you raise a followup PR to fix it, targeting the main
branch?
…nnected.
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