Remove Dashboard View change and UA Dashboard View components. #7581
Conversation
|
Build files for d88afa2 are ready:
|
|
Size Change: -6.73 kB (0%) Total Size: 1.4 MB
鈩癸笍 View Unchanged
|
There was a problem hiding this comment.
Hey @tofumatt, nice work here so far. There are, though, a couple of points from the IB that have been missed:
- Remove the
Newbadges added to indicate new GA4 metrics.- The Analytics module SettingsView should be restructured following this Figma screen.
As per my separate comment, it looks like we can also remove the remaining usage of GA4_DASHBOARD_VIEW_NOTIFICATION_ID.
Thanks for the additional info in Relevant technical choices - from what I can see, keeping isGA4DashboardView() makes sense for now as we still do rely on its remaining logic, although arguably it should be renamed. We've still got plenty more tidying up and removing of UA features to come though, so I think it can happily wait for another issue, and from what I can see the remaining stories you mentioned can too.
| @@ -52,7 +52,4 @@ export const SETUP_FLOW_MODE_GA4_TRANSITIONAL = 'ga4-transitional'; | |||
| export const PROPERTY_TYPE_UA = 'ua'; | |||
| export const PROPERTY_TYPE_GA4 = 'ga4'; | |||
|
|
|||
| export const DASHBOARD_VIEW_GA4 = 'google-analytics-4'; | |||
| export const DASHBOARD_VIEW_UA = 'universal-analytics'; | |||
|
|
|||
| export const GA4_DASHBOARD_VIEW_NOTIFICATION_ID = 'switch-ga4-dashboard-view'; | |||
There was a problem hiding this comment.
It looks like this GA4_DASHBOARD_VIEW_NOTIFICATION_ID constant and the remaining use of it can also be removed...
|
Thanks @techanvil! Changes made and removed those new badges, so this should be good-to-go now 馃憤馃徎 |
There was a problem hiding this comment.
Thanks @tofumatt - however this one point from the IB is still technically outstanding (specifically, its initial sub-point):
- The Analytics module
SettingsViewshould be restructured following this Figma screen.
- In
assets/js/modules/analytics/components/settings/SettingsView.js:
- Add a new setting meta item at the first position before all other settings, labelled
Account, which should display the account ID. This can be copied over fromassets/js/modules/analytics/components/settings/GA4SettingsView.js.
I do realise it doesn't directly correlate to the AC, but it has been specced - having said that you did approve the IB, so if you feel it's out of scope here, I'd be happy to skip this part. What do you think?
|
Oh, right, I meant to cover that here in the technical notes. We have a separate issue for that here: #6821. I think I meant to mention it in my IB Review and totally forgot 馃う馃徎 I think we should omit it from this issue though. It's pretty unrelated and as discussed in that IB might warrant a new selector, etc., so we should do that separately. |
Ah, cheers, I thought that was just a timeout. I've fixed it, now is good-to-go 馃槃 |
There was a problem hiding this comment.
Hey @tofumatt, thanks for updating the VRT tests. Sorry to have to send this back again, I had thought it would indeed be good to go. However, I've noticed that you've removed some tests as part of your recent merge of develop into this branch - see this diff.
The thing is, the logic these test cases are testing still exists in the codebase - it's part of the recently merged PR #7590.
So, unless we're also removing this logic here - which I think would be out of scope, and I don't feel entirely confident it would be safe to do so just yet, it would need a good bit of unpicking to check if these cases are not longer reachable - I think we should keep the tests too. It looks like they'd need a bit of a tweak to use an alternative way to modify the return value of isGA4DashboardView(), with the explicit dashboardView setting out of the picture, but beyond that I think they are good to keep. What do you think?
|
Sure thing, I'll make the changes to those tests/logic and restore them 馃憤馃徎 |
Summary
Addresses issue:
Relevant technical choices
I opted not to remove the
isGA4DashboardViewselector itself but hardcode it totrue, just to cut down on PR size since it's not explicitly in the ACs, but I can add that to this one if needed.This removes all Dashboard View settings in PHP/JS and the UI to switch between the settings and to view it. The Dashboard view is now always GA4.
There are a few Storybook stories that still reference UA, which I thought was helpful as part of this PR to confirm the change, but that could ultimately be removed in this PR if desired. But otherwise they're fine to remove during the
ga4Reportingfeature flag removal.From what I can see this satisfies all the ACs and works well in my testing鈥攊t might even go a bit beyond the ACs, but I figured removing as much code where straightforward is good. Let me know if you'd like less or more removed 馃槅
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