-
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
Implement the “no audiences” banner happy path view #8155
Comments
I've moved this back to Backlog as the final in-progress changes to the design doc, relating to audience caching, will probably affect the AC for this one |
The audience caching aspect of the design doc has been sufficiently finalised, and I've moved this back to AC. |
Hey @ankitrox, Thanks for drafting the IB. A few suggestions: Instead of baking this inside The implication is that we need to render Also instead of having two banners, we can keep the logic in a single one, since the difference is only copy and link. Should be manageable in one place. Finally, some styling and layout ideas can be borrowed from Cheers. |
Thank you for reviewing the IB @kuasha420 Adding the However, I think we should keep the Assigning this to you for re-review. Thanks again! |
Hi @ankitrox, Thank you for updating the IB. Looking pretty good. Just a couple of points:
Cheers. |
Thank you @kuasha420 I have made the necessary changes as suggested, but kept the configurable and non-configurable components separate with the changes in name as suggested. Assigning this to you for re-review. Cheers! |
Thanks @ankitrox. IB LGTM. ✅ |
Hi @kuasha420 . As discussed internally, this issue is back with you to review if its PR needs any changes post the reversal of pivot reports. The |
QA Update
|
Hi @kelvinballoo, thanks for flagging this. These points are known issues, and we will be addressing them via #8145, ensuring the list of selected audiences is updated following the sync. |
QA Update
|
Hi @kelvinballoo, thanks for reviewing this and raising these issues.
Good spot, and agreed, it will be better UX as you've described. We do already have an issue for this, #8875 - it's a P2 though as it's not essential for launch.
Another good point to raise. I was debating whether to leave this aspect in. I'm pretty sure I was at one point able to remove all audiences including the "All users" default. However in recent testing, it's not possible to remove it. I figured that we can leave this edge case in for now as it's a small addition and will be a robust way to handle the scenario if it does occur again for one reason or another. In the meantime, testing it programmatically by using the snippet in the QAB is the way to go here. |
QA Update ✅Thanks @techanvil for checking. We are good to go on this issue.
|
Feature Description
Implement the banners shown in the scenarios where all of the selected audiences have been archived in Analytics. This includes both variants of the banner that will be shown to the primary user.
The error state and the secondary user variants of the banner will be implemented separately, see #8190 and #8577.
See no audiences banner in the design doc.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
You can deactivate this widget in Settings.
You can deactivate this widget in Settings.
Implementation Brief
Create following 4 new components inside
assets/js/modules/analytics-4/components/audience-segmentation/dashboard/
NoAudienceBannerWidget
- Component responsible for returning the appropriate component, eitherNonConfigurableNoAudienceBannerContent
orConfigurableNoAudienceBannerContent
. Component should accept theWidget
andWidgetNull
just likeAudienceTilesWidget
component mentioned above.ConfigurableNoAudienceBannerContent
- This component is respondible for rendering the "no audience" banner when dashboard is not in view only context.NonConfigurableNoAudienceBannerContent
- This component is respondible for rendering the "no audience" banner when dashboard is in view only context.AudienceTiles
- This component will be respondible to display the configured audiences tiles. This will only be called from insideAudienceTilesWidget
component, when we made a successful check that at least one of the configured audiences exist in the Google analytics.AudienceTilesWidget
component. AcceptsWidget
andWidgetNull
props.WidgetNull
should be passed already, we just need to destructure it in the component.AudienceTiles
component as mentioned inAudienceTiles
component brief.WidgetNull
from the component if none of the configured audiences present in the available audiences. This implies that we need to display theNoAudienceBannerWidget
component described below.NoAudienceBannerWidget
component. AcceptsWidget
andWidgetNull
prop.configuredAudiences
) and the available audiences (getAvailableAudiences
) usinguseSelect
hook, referAudienceTilesWidget
component for the same.WidgetNull
from component if any of the audience inconfiguredAudiences
is available insidegetAvailableAudiences
.getAvailableAudiences
:ConfigurableNoAudienceBannerContent
component.NonConfigurableNoAudienceBannerContent
banner component.ConfigurableNoAudienceBannerContent
component. AcceptsWidget
prop.getAvailableAudiences
, then the banner should match the figma design.Widget
component.AUDIENCE_SELECTION_PANEL_OPENED_KEY
needs to be set totrue
inCORE_UI
data store as can be seen here.Admin Settings > Visitor groups
.getAvailableAudiences
, we should display a different text in the banner as shown in figma design.NonConfigurableNoAudienceBannerContent
component. AcceptsWidget
prop.Widget
component and should match the figma design.AudienceTiles
component. Accepts Widget prop.In
assets/js/modules/analytics-4/index.js
, register a new widget usingwidgets.registerWidget(...)
afteranalyticsAudienceTiles
.NoAudienceBannerWidget
.Most of the styling from Enhance/8156 connect analytics cta #8798 can be reused for the banner as those are mostly similar in aesthetics.
Ensure that mobile design is matching the figma design
Cell
component within the widget to stack the components vertically for the mobile design.Grid
andCell
components to take into account about the mobile design.Test Coverage
NoAudienceBannerWidget
,ConfigurableNoAudienceBannerContent
andNonConfigurableNoAudienceBannerContent
components.AudienceTilesWidget
component as we need to test whether it is returning appropriate components from it based on audiences availability and dashboard view context.AudienceTilesWidget
component as we need to test whether it is returningWidgetNull
when no configured audiences available anymore.QA Brief
InfoNoticeWidget
always being there, it is being tackled in a separate issue.)TIP: It may be necessary to sync available audiences after archiving/deleting them in Analytics. You can force this using the following snippet.
For Quickly testing the banner. You can set configured audience to a non-existent one.
And for seeing the alternate version, sett the available audiences to an empty array so no audience is available.
Changelog entry
The text was updated successfully, but these errors were encountered: