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

Consider re-positioning settings in GA SettingsView #6821

Closed
nfmohit opened this issue Apr 5, 2023 · 7 comments
Closed

Consider re-positioning settings in GA SettingsView #6821

nfmohit opened this issue Apr 5, 2023 · 7 comments
Labels
Good First Issue Good first issue for new engineers Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Apr 5, 2023

Feature Description

If we take a look at the final state of the Analytics module settings in this Figma mockup, it can be seen that the Account setting is on its own row. Do we think it'd be a good idea to implement this positioning consistently in Analytics SettingsEdit and SettingsView?

image

So, as for positioning:

  1. GA account setting at the first row.
  2. GA4 settings at the second & third rows.
  3. UA settings at the fourth & fifth rows (until it is removed).
  4. Other settings as they are today.

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

Acceptance criteria

  • In Analytics Settings View (eg. SettingsView), the Google Analytics Account should appear on its own line, separate from the property, measurement ID, etc, similar to the Figma mocks:

CleanShot 2023-05-05 at 17 45 14

  • The new account row should have an added link to Edit in Analytics as well, as shown in the design which opens https://analytics.google.com/analytics/web/#/a{accountID}p{propertyID}/admin/account/settings in a new tab/window

Implementation Brief

  • For the settings view, move the "Account" setting (DisplayView) into its own section (eg. its own <div className="googlesitekit-settings-module__meta-items">) like so:
<div className="googlesitekit-settings-module__meta-items">
	{ ga4ReportingEnabled && (
		<div className="googlesitekit-settings-module__meta-item">
			<h5 className="googlesitekit-settings-module__meta-item-type">
				{ __( 'Account', 'google-site-kit' ) }
			</h5>
			<p className="googlesitekit-settings-module__meta-item-data">
				<DisplaySetting value={ accountID } />
			</p>
		</div>
	) }
</div>
  • Add an "Edit in Analytics" link next to the Account in settings view that goes to this URL, for the respective account ID: https://analytics.google.com/analytics/web/?pli=1#/a${GA4_ACCOUNT_ID}p${GA4_PROPERTY_ID}/admin/account/settings

Test Coverage

  • No new tests needed, but VRTs will need updating.

QA Brief

  • Set up Site Kit with the Analytics module.
  • Go to Site Kit settings and open the settings view screen for Analytics.
  • Verify that the account ID appears in its own row.
  • Verify that the associated edit link works as described in the ACs.

Changelog entry

  • Adjust the layout of the Analytics settings view.
@nfmohit nfmohit added Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels Apr 5, 2023
@eclarke1 eclarke1 added the P1 Medium priority label Apr 6, 2023
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Apr 7, 2023
@aaemnnosttv aaemnnosttv added P2 Low priority and removed P1 Medium priority labels Apr 17, 2023
@bethanylang bethanylang added P1 Medium priority and removed P2 Low priority labels Apr 19, 2023
@tofumatt tofumatt self-assigned this May 5, 2023
@tofumatt tofumatt changed the title Consider re-positioning settings in GA SettingsEdit and SettingsView Consider re-positioning settings in GA SettingsView May 14, 2023
@tofumatt
Copy link
Collaborator

Had another think on this and I don't think we should move this in the "edit" mode, especially as it isn't that way in Figma. But moving the view setting and adding a link to edit that account in Analytics is 👍🏻

@tofumatt tofumatt assigned tofumatt and unassigned tofumatt May 14, 2023
@tofumatt tofumatt added the Good First Issue Good first issue for new engineers label May 19, 2023
@aaemnnosttv aaemnnosttv self-assigned this May 24, 2023
@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented May 24, 2023

Thanks @tofumatt – the IB includes a code snippet which seems to have a feature flag condition within it which isn't part of the AC here. I think the first point of the IB should be sufficient for this simple change, let's remove the code which shouldn't be necessary but also seems to be incorrect.

  • Add an "Edit in Analytics" link next to the Account in settings view that goes to this URL, for the respective account ID: https://analytics.google.com/analytics/web/?pli=1#/a${GA4_ACCOUNT_ID}p${GA4_PROPERTY_ID}/admin/account/settings (this should use the getServiceURL selector for Analytics)

Let's simplify this to avoid confusion to call for using getServiceURL (as you mentioned) on the analytics store (since this selector doesn't exist on the GA4 store) with the respective relative path, i.e. omitting all the other details which are provided automatically by the selector.

Also – this change seems simple enough as a 7, or even less but I'm guessing this would be with the Exp: SP label?

@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned aaemnnosttv May 24, 2023
@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Sep 4, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Sep 4, 2023

Indeed, this is a 7 with the Exp: SP label.

I've updated the URL creation not to use the old analytics module, I missed that because both data stores have a getServiceEntityAccessURL URL which the other "Edit in Analytics" link uses.

The GA4SettingsView already uses it here:

const editDataStreamSettingsURL = useSelect( ( select ) =>
select( MODULES_ANALYTICS_4 ).getServiceEntityAccessURL()
);
, so that can serve as a reference to create another selector for the URL mentioned in the IB. I think we should add that part in as well, so I've left it in the IB even though it's missing from the ACs. We should amend the ACs if needed, but the edit link is part of the design and we should add it in during this issue, I think, rather than create a new issue for it.

@aaemnnosttv
Copy link
Collaborator

Thanks @tofumatt!

We shouldn't really be using getServiceEntityAccessURL in this way, it's only intended to be used to request access to an entity that you don't have access/permissions for yet, and defining it in the first place enables this kind of behavior in the dashboard. Even if the end result is the same, we shouldn't use it for this.

It might be worth defining a new getServiceAdminURL (if only for GA4) to build URLs relative to //analytics.google.com/analytics/web/#/a{accountID}p{propertyID}/admin if we have multiple uses for it, (similar to getServiceReportURL) but for now constructing with getServiceURL is probably fine.

I'll amend the AC to include a reference to the edit link 👍

@tofumatt tofumatt removed their assignment Sep 18, 2023
@tofumatt
Copy link
Collaborator

I think the IB here is good now, I don't see a reference to getServiceURL in it so should be good-to-go 🤔

@aaemnnosttv
Copy link
Collaborator

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment Nov 13, 2023
@nfmohit nfmohit self-assigned this Nov 20, 2023
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Nov 21, 2023
@zutigrm zutigrm assigned zutigrm and nfmohit and unassigned zutigrm Nov 21, 2023
@nfmohit nfmohit assigned zutigrm and unassigned nfmohit Nov 22, 2023
@zutigrm zutigrm removed their assignment Nov 22, 2023
@techanvil techanvil self-assigned this Nov 22, 2023
techanvil added a commit that referenced this issue Nov 22, 2023
…ings-repositioning

Re-position settings in Analytics `SettingsView`
@techanvil techanvil removed their assignment Nov 22, 2023
@wpdarren wpdarren self-assigned this Nov 22, 2023
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • In Analytics Settings View the Account appears on its own line, separate from the property, measurement ID, etc
  • The new account row has an added link to Edit in Analytics, which opens https://analytics.google.com/analytics/web/#/a{accountID}p{propertyID}/admin/account/settings in a new tab.
  • Tested this on mobile and other smaller viewports to make sure there are no UX/UI issues.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

10 participants