google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 284 forks source link

Create Ads placeholder reporting widget #8559

Closed aaemnnosttv closed 4 months ago

aaemnnosttv commented 5 months ago

Feature Description

With Ads connected via PAX, the Ads module should conditionally register a new widget on the dashboard which be populated by a new PAX widget, the contents of are WIP but delegated to the app.

This widget should appear in the "Traffic" section, and will not be accessible to users in the view-only dashboard context.


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

tofumatt commented 5 months ago

I made some changes here, mainly adding more clarity around the widget behaviour/expectations for the component, but also removing the need to pass in the config or services, since I think the PAX component itself can handle that (they'll be consistent across the site as I understand it).

@10upsimon Let me know if this looks good, if so feel free to move it right to IB πŸ‘πŸ»

10upsimon commented 5 months ago

Thanks @tofumatt those changes look good to me, moving to IB

eugene-manuilov commented 5 months ago

IB βœ”οΈ

aaemnnosttv commented 5 months ago

I tweaked the definition here to include the use of whenActive plus a new HOC for guarding based on a granted scope, since it is conditional for Ads at this point. The estimate was also bumped to accommodate for this.

wpdarren commented 4 months ago

QA Update: ❌

@tofumatt a few observations:

  1. I see the widget but nothing is getting loaded in it. There's a reload UX/UI and even when I refresh nothing happens. There's also a huge console error. Part of the error message is below (it's a long one so not all of it can be pasted)

To note: I have successfully set up my campaign, clicked on the update button to connect the Ads module.

POST https://ads.google.com/aw_express/embedding/_/rpc/EmbeddedMiniDashboardViewModelService/LoadViewModel?authuser&acx-v-bv=awn_express_auto_20240508-0457_RC000&acx-v clt=1715737524077&rpcTrackingId=EmbeddedMiniDashboardViewModelService.LoadViewModel%3A1 500 (Internal Server Error)

image

2. If Analytics is not connected then the widget does not appear. We could have a scenario where the user does not have this module set up, so, should the widget appear? I suspect it should since Ads and Analytics are not directly impacted for setting up an ads account and campaign. The widget actually appears, tested on another site. Please ignore.

tofumatt commented 4 months ago

Turns out that bug was due to the date range code not being merged yet.

Now that #8687 is merged this should work, so moving back to QA πŸ‘πŸ»

wpdarren commented 4 months ago

QA Update: ⚠️

@zutigrm The UX/UI of the widget doesn't look very good.

There's lots of white space, wrapping issues with text, etc. Interested in your thoughts considering its a pilot.

image

zutigrm commented 4 months ago

@wpdarren thanks for your observation. As @tofumatt mentioned in the slack we can't control the widget, since it is third party and it can adjusted only on the Ads team side

wpdarren commented 4 months ago

QA Update: βœ…

Verified:

Note that there are numerous UX/UI issues with the reporting widget, but we will identify these to the Ads team to see if they can be fixed and make the widget more user-friendly.

image