-
Notifications
You must be signed in to change notification settings - Fork 286
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
Enhancement/#8144 - Handle “new visitors” and “returning visitors” to avoid the “partial data” state #9232
Enhancement/#8144 - Handle “new visitors” and “returning visitors” to avoid the “partial data” state #9232
Conversation
Build files for 357eea2 have been deleted. |
Size Change: +8.4 kB (+0.47%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
.../analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/index.stories.js
Show resolved
Hide resolved
.../analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/index.stories.js
Show resolved
Hide resolved
.../analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/index.stories.js
Show resolved
Hide resolved
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.
Hey @hussain-t, the PR and code looks good, great use of a hook to split out the complex queries from this already complex component.
I've add a couple of small comments below, LMK your thoughts.
.../analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/index.stories.js
Show resolved
Hide resolved
...les/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/index.test.js
Outdated
Show resolved
Hide resolved
Thanks @hussain-t, I resolved the conflicts with the useInViewSelect work as I was working on that ticket and moving to MR once the checks pass. |
@@ -311,7 +311,7 @@ ViewOnlyNoDimensions.args = { | |||
viewContext: VIEW_CONTEXT_MAIN_DASHBOARD_VIEW_ONLY, | |||
setupRegistry: ( registry ) => { | |||
registry.dispatch( MODULES_ANALYTICS_4 ).setSettings( { | |||
availableAudiences: null, | |||
availableAudiences: [], |
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.
Mocking with a null
value breaks the story.
site-kit-wp/assets/js/modules/analytics-4/datastore/audiences.js
Lines 714 to 716 in 7ce10f7
const audience = availableAudiences.find( | |
( { name } ) => name === audienceResourceName | |
); |
Thanks for the valuable feedback, @techanvil. I've addressed them. |
…rableSiteKitAndOtherAudiences.
…rtial data 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.
Hi @hussain-t, thanks for the update.
However, when I tested it, filtering for audienceResourceName
or newVsReturning
in the network tab to verify the correct reports were being made, I found it still wasn't behaving as expected.
Digging in I realised this IB point hadn't been covered and it had simply escaped me the first time round:
Fetch the top cities, top content and top content titles for
siteKitAudienceResourceNames
separately withdimensionFilters
set tonewVsReturning
.
I also noticed getConfiguredSiteKitAndOtherAudiences()
was misnamed so giving unexpected results when trying to use it here.
So, it turned out to still need a fair bit of work. To help move this along I've got stuck into this myself and have got it to a point where it's working in my testing.
I've pushed the changes, please take a look, and if you're happy then wrap it up from here, there are a few JS tests that need to be fixed, and a failing VRT test, once these are done it should be good to go 🤞
Thanks so much for the fixes, @techanvil. I have made a couple of minor fixes and fixed the failing tests. Please note that VRT keeps failing randomly upon several attempts. |
No problem @hussain-t. I've also taken a look at the VRT and realised it needed an update so I've pushed that as well to move this along. I also updated a JS test which was sometimes failing for me locally. |
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 now LGTM, thanks @hussain-t.
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