Skip to content
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

Unified Dashboard: Register widgets in new context "Traffic" #4149

Closed
tofumatt opened this issue Sep 28, 2021 · 14 comments
Closed

Unified Dashboard: Register widgets in new context "Traffic" #4149

tofumatt opened this issue Sep 28, 2021 · 14 comments
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Milestone

Comments

@tofumatt
Copy link
Collaborator

tofumatt commented Sep 28, 2021

Feature Description

As per the design doc, there should add the following widgets to the Traffic widget area:


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The existing DashboardAllTrafficWidget should be registered as a full-width widget with a priority of "1" (the first widget in the area) in the following Unified Dashboard widget areas:
    • AREA_MAIN_DASHBOARD_TRAFFIC_PRIMARY
    • AREA_ENTITY_DASHBOARD_TRAFFIC_PRIMARY
  • The widget registration should only take place when the feature flag unifiedDashboard is enabled. Technically we don't need this because without this feature flag enabled this widget area will not be displayed, but it serves to clearly mark our intentions in the code so makes sense to mark.
  • The widget should behave as it does today—showing site-wide data on the Main Dashboard and Entity-specific data on the Entity Dashboard.

Implementation Brief

  • Inside assets/js/modules/analytics/index.js:
    • Import the AREA_MAIN_DASHBOARD_TRAFFIC_PRIMARY and AREA_ENTITY_DASHBOARD_TRAFFIC_PRIMARY widget areas from assets/js/googlesitekit/widgets/default-areas.js.
    • Use isFeatureEnabled to determine if the unifiedDashboard flag is enabled.
    • If it is, make an additional call to widgets.registerWidget:
      widgets.registerWidget(
      	'analyticsAllTraffic',
      	{
      		Component: DashboardAllTrafficWidget,
      		width: widgets.WIDGET_WIDTHS.FULL,
      		priority: 1,
      		wrapWidget: false,
      	},
      	[ `AREA_MAIN_DASHBOARD_TRAFFIC_PRIMARY`, `AREA_ENTITY_DASHBOARD_TRAFFIC_PRIMARY` ]
      );

Test Coverage

  • No new tests required.

QA Brief

  • Enable the unifiedDashboard feature flag using the tester plugin.
  • Navigate to the dashboard and verify that the All Traffic Widget is present.
  • Navigate to the entity dashboard (add a permaLink query param, e.g. /wp-admin/admin.php?page=googlesitekit-dashboard&permaLink=test) and verify that the All Traffic Widget is there as well.

Changelog entry

  • Register all traffic widget in traffic sections of the unified dashboard.
@tofumatt tofumatt added P1 Medium priority Type: Enhancement Improvement of an existing feature labels Sep 28, 2021
@felixarntz
Copy link
Member

felixarntz commented Sep 28, 2021

@tofumatt Based on https://docs.google.com/document/d/1OnVv9P5Hbz-owVnzawoO-o37GLTFhyeIyBO6K5YZr38/edit?disco=AAAAP5ukqmA, I thought we would register the new widgets when they're ready, instead of blocking the general widget registration issues from them. Is that not what you meant? I'm thinking we could already move this forward to at least have the area appear correctly (with only the All Traffic widget initially), and then the Search Funnel issue would be responsible for registering that new widget in the area later.

Also, based on the issue description of #4122 and #4123, I think the related issue here is #4123, not #4122.

@tofumatt
Copy link
Collaborator Author

Indeed, I've modified the new widgets to specify that they should add their registration when ready so this is no longer blocked by the new widget. 👍🏻

@tofumatt tofumatt removed their assignment Sep 28, 2021
@aaemnnosttv aaemnnosttv self-assigned this Sep 30, 2021
@aaemnnosttv
Copy link
Collaborator

IB ✅

@johnPhillips
Copy link
Contributor

@aaemnnosttv I think, looking again at the AC, that this might inadvertently be blocked by #4122?

I've done the IB and PR to include the All Traffic Widget on the basis that it will be straightforward to register the Overall Page Metrics widget as part of that issue. Hope that's ok?

@felixarntz
Copy link
Member

@johnPhillips @aaemnnosttv That should be good with only the All Traffic widget for now. #4122 is going to cover registering the new widget in here once it's ready. The focus of this issue here is to set up everything in that widget context which we already have.

@fhollis fhollis added this to the Sprint 59 milestone Oct 1, 2021
@aaemnnosttv aaemnnosttv removed their assignment Oct 4, 2021
@wpdarren wpdarren self-assigned this Oct 4, 2021
@wpdarren
Copy link
Collaborator

wpdarren commented Oct 4, 2021

QA Update: ⚠️

@tofumatt I'd just like to highlight two observations.

  1. With unifiedDashboard feature flag enabled, and when AdSense is not activated, there is a modal for Additional Permissions Required related to AdSense. To get by this, I had to enable AdSense on the tester plugin and connect. Is this expected at this stage?

image

  1. With unifiedDashboard feature flag enabled, I do see the All Traffic widget when on Site Kit dashboard wp-admin/admin.php?page=googlesitekit-dashboard and underneath that I see the two AdSense widgets. When I add a permaLink query /wp-admin/admin.php?page=googlesitekit-dashboard&permaLink=test I still see the All Traffic widget but no longer see the two AdSense widgets. Is this also expected at this stage? Did not want to assume.

image

@felixarntz
Copy link
Member

@wpdarren Your first point looks valid, this shouldn't be happening, so needs to be fixed. Your second point is expected, when looking at the results for a single URL (entity dashboard), the AdSense widgets should not be present, as those only work for the entire site level.

@tofumatt That AdSense widget probably needs to receive the whenActive treatment, which I assume it didn't have before just because it was only appearing on the AdSense module page, which didn't appear without the module active in the first place. Let's add the whenActive use here, with the following details: The widget should only have a fallback component for when the module is active but not connected - i.e. if AdSense is not active, it doesn't show at all, and if AdSense is not fully connected, it shows the blue CTA to "complete module activation".

@felixarntz felixarntz assigned tofumatt and unassigned wpdarren Oct 4, 2021
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Oct 6, 2021
@aaemnnosttv aaemnnosttv self-assigned this Oct 6, 2021
@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned aaemnnosttv Oct 6, 2021
@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Oct 6, 2021
@aaemnnosttv
Copy link
Collaborator

@wpdarren this is ready for another pass on main whenever you are 👍

@aaemnnosttv aaemnnosttv assigned wpdarren and unassigned aaemnnosttv Oct 6, 2021
@wpdarren
Copy link
Collaborator

wpdarren commented Oct 7, 2021

QA Update: ⚠️

@tofumatt @aaemnnosttv when I first set up Site Kit, it redirects me to the dashboard as expected. At this point Analytics is not connected. I see a blank page with no widgets loading. There's a warning message in the console. It's possible that this is expected but wanted to check with you.

image

When I go to Site Kit settings and connect Analytics, the All Traffic widget does appear on the dashboard. I also go to /wp-admin/admin.php?page=googlesitekit-dashboard&permaLink=test and the widget also appears here too.

Looks like this is a pass but would appreciate your thoughts on the initial behaviour.

@tofumatt
Copy link
Collaborator Author

tofumatt commented Oct 7, 2021

Looks like that's technically a bug, because I suspect it would cause Analytics not to load properly.

Seems like we should be conditionally registering those widgets only once in the correct areas, based on the Unified Dashboard feature flag. I'll create a quick follow-up PR.

@tofumatt tofumatt assigned tofumatt and unassigned wpdarren Oct 7, 2021
@tofumatt tofumatt removed their assignment Oct 7, 2021
@aaemnnosttv
Copy link
Collaborator

Back to you for another pass @wpdarren 👍

@wpdarren
Copy link
Collaborator

wpdarren commented Oct 7, 2021

@tofumatt I left a message in Slack. I am still seeing a blank page in Site Kit dashboard but the warning has disappeared. For sanity check, could you confirm this is expected. When I connect Analytics in Site Kit settings, this does pass per the QAB.

@aaemnnosttv
Copy link
Collaborator

@wpdarren I believe that is expected but only when the unifiedDashboard flag is enabled. When not enabled, the dashboards should look as usual.

@tofumatt can you confirm?

@wpdarren
Copy link
Collaborator

wpdarren commented Oct 7, 2021

@aaemnnosttv @tofumatt thank you for the clarification on here/Slack.

QA Update: ✅

  • When the unifiedDashboard feature flag is enabled, the All Traffic Widget is present on Site Kit dashboard.
  • When a permaLink query param is used the All Traffic Widget is displayed as well.
  • When the unifiedDashboard feature flag is disabled, the dashboard loads as normal.

@wpdarren wpdarren removed their assignment Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants