-
Notifications
You must be signed in to change notification settings - Fork 274
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
Add Site Health section for sitekit created audiences. #8710
base: develop
Are you sure you want to change the base?
Conversation
Build files for 343ce5c are ready:
|
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.
Thanks @ankitrox, this is a good start. I've left a few comments, please take a look.
includes/Modules/Analytics_4.php
Outdated
|
||
$debug_fields['analytics_4_site_kit_audience'] = array( | ||
'label' => __( 'Analytics site created audiences', 'google-site-kit' ), | ||
'value' => $site_kit_audiences, |
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, we missed this when writing the IB, but please can you ensure the comma is translated for the value
version of the string? For example:
site-kit-wp/includes/Modules/Analytics_4.php
Lines 483 to 492 in e2daae9
'value' => empty( $settings['availableCustomDimensions'] ) | |
? __( 'None', 'google-site-kit' ) | |
: join( | |
/* translators: used between list items, there is a space after the comma */ | |
__( ', ', 'google-site-kit' ), | |
$settings['availableCustomDimensions'] | |
), | |
'debug' => empty( $settings['availableCustomDimensions'] ) | |
? 'none' | |
: join( ', ', $settings['availableCustomDimensions'] ), |
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, thanks for the update. I've left a few more minor comments, please take a look.
includes/Modules/Analytics_4.php
Outdated
$created_audiences = array(); | ||
|
||
// Ensure that audiences are available, otherwise return an empty array. | ||
if ( empty( $audiences ) || ! is_array( $audiences ) ) { | ||
return $created_audiences; | ||
} | ||
|
||
$created_audiences = array_filter( $audiences, fn( $audience ) => ! empty( $audience['audienceType'] ) && ( 'SITE_KIT_AUDIENCE' === $audience['audienceType'] ) ); | ||
|
||
if ( empty( $created_audiences ) ) { | ||
return array(); | ||
} | ||
|
||
return wp_list_pluck( $created_audiences, 'displayName' ); |
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'd suggest a small refactor here, note that wp_list_pluck()
will return an empty array when passed one so there's no need for the explicit check:
$created_audiences = array(); | |
// Ensure that audiences are available, otherwise return an empty array. | |
if ( empty( $audiences ) || ! is_array( $audiences ) ) { | |
return $created_audiences; | |
} | |
$created_audiences = array_filter( $audiences, fn( $audience ) => ! empty( $audience['audienceType'] ) && ( 'SITE_KIT_AUDIENCE' === $audience['audienceType'] ) ); | |
if ( empty( $created_audiences ) ) { | |
return array(); | |
} | |
return wp_list_pluck( $created_audiences, 'displayName' ); | |
// Ensure that audiences are available, otherwise return an empty array. | |
if ( empty( $audiences ) || ! is_array( $audiences ) ) { | |
return array(); | |
} | |
$site_kit_audiences = array_filter( $audiences, fn( $audience ) => ! empty( $audience['audienceType'] ) && ( 'SITE_KIT_AUDIENCE' === $audience['audienceType'] ) ); | |
return wp_list_pluck( $site_kit_audiences, 'displayName' ); |
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.
Thanks @ankitrox! There are two more comments to address - one is a new one that I meant to add before, the other is one that was missed from my previous review: #8710 (comment)
Co-authored-by: Tom Rees-Herdman <tom.rees-herdman@10up.com>
…ub.com:google/site-kit-wp into enhancement/8181-site-health-created-audiences.
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