-
Notifications
You must be signed in to change notification settings - Fork 278
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
Implement the happy path for creating audiences and the custom dimension via the Setup CTA Banner. #8694
Conversation
Pending additional conditionality.
Outline the remaining tests.
…created audiences.
Build files for 51540e5 have been deleted. |
Size Change: +24.5 kB (+1.7%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
assets/sass/components/dashboard/_googlesitekit-subtle-notification.scss
Show resolved
Hide resolved
assets/sass/components/dashboard/_googlesitekit-subtle-notification.scss
Outdated
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.
Excellent work, @techanvil 🎖️
I've left a few minor comments. Could you take a look?
...onents/audience-segmentation/dashboard/AudienceSegmentationSetupSuccessSubtleNotification.js
Outdated
Show resolved
Hide resolved
assets/sass/components/dashboard/_googlesitekit-subtle-notification.scss
Outdated
Show resolved
Hide resolved
assets/sass/components/dashboard/_googlesitekit-subtle-notification.scss
Outdated
Show resolved
Hide resolved
Co-authored-by: Hussain Thajutheen <hussain.thajutheen@10up.com>
...analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.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.
Excellent work, @techanvil. LGTM 🥇 👍
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.
Thank you @techanvil, amazing work here. LGTM. 🎉
Summary
Addresses issue:
Relevant technical choices
As discussed in a recent standup, I realised during implementation that reusing the partial data infrastructure as described in the IB is not such a good fit here, so instead I've taken the approach of retrieving a regular Analytics report for the 90 day range to determine whether data is available. This does mean we can't take advantage of a cached data availability state which is available on page load in order to avoid layout shifts when showing the banner; I will create a followup issue to investigate further options on that front.
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