Provide link in Analytics settings panel to update GA4 configuration and improve UI for related UA link #3766

danielgent · 2021-07-27T11:41:13Z

Summary

Addresses issue #3702

Relevant technical choices

Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

github-actions · 2021-07-27T11:46:44Z

Size Change: -2.01 kB (0%)

Total Size: 1.08 MB

Filename	Size	Change
`./dist/assets/js/googlesitekit-activation.********************.js`	35.7 kB	-45 B (0%)
`./dist/assets/js/googlesitekit-adminbar.********************.js`	28.1 kB	-96 B (0%)
`./dist/assets/js/googlesitekit-api.********************.js`	9.2 kB	-57 B (-1%)
`./dist/assets/js/googlesitekit-dashboard-details.********************.js`	41.3 kB	-138 B (0%)
`./dist/assets/js/googlesitekit-dashboard-splash.********************.js`	49.8 kB	-65 B (0%)
`./dist/assets/js/googlesitekit-dashboard.********************.js`	41.6 kB	-69 B (0%)
`./dist/assets/js/googlesitekit-data.********************.js`	1.65 kB	+5 B (0%)
`./dist/assets/js/googlesitekit-datastore-forms.********************.js`	8.92 kB	-37 B (0%)
`./dist/assets/js/googlesitekit-datastore-location.********************.js`	2.03 kB	+3 B (0%)
`./dist/assets/js/googlesitekit-datastore-site.********************.js`	13.4 kB	-51 B (0%)
`./dist/assets/js/googlesitekit-datastore-ui.********************.js`	8.89 kB	-36 B (0%)
`./dist/assets/js/googlesitekit-datastore-user.********************.js`	20.3 kB	-45 B (0%)
`./dist/assets/js/googlesitekit-idea-hub-notice.********************.js`	2.42 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-module.********************.js`	41.4 kB	-96 B (0%)
`./dist/assets/js/googlesitekit-modules-adsense.********************.js`	44.1 kB	-57 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-4.********************.js`	19.1 kB	-33 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics.********************.js`	65.6 kB	-243 B (0%)
`./dist/assets/js/googlesitekit-modules-idea-hub.********************.js`	39.3 kB	-234 B (-1%)
`./dist/assets/js/googlesitekit-modules-optimize.********************.js`	17 kB	-61 B (0%)
`./dist/assets/js/googlesitekit-modules-pagespeed-insights.********************.js`	16.5 kB	-27 B (0%)
`./dist/assets/js/googlesitekit-modules-search-console.********************.js`	28.7 kB	-39 B (0%)
`./dist/assets/js/googlesitekit-modules-tagmanager.********************.js`	30.1 kB	-81 B (0%)
`./dist/assets/js/googlesitekit-modules.********************.js`	16.6 kB	-54 B (0%)
`./dist/assets/js/googlesitekit-settings.********************.js`	45.9 kB	-116 B (0%)
`./dist/assets/js/googlesitekit-user-input.********************.js`	42.7 kB	-140 B (0%)
`./dist/assets/js/googlesitekit-vendor.********************.js`	314 kB	-26 B (0%)
`./dist/assets/js/googlesitekit-widgets.********************.js`	11.5 kB	-56 B (0%)
`./dist/assets/js/googlesitekit-wp-dashboard.********************.js`	29.9 kB	-118 B (0%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/admin.css`	38.1 kB
`./dist/assets/css/adminbar.css`	7.95 kB
`./dist/assets/css/wpdashboard.css`	4.55 kB
`./dist/assets/js/analytics-advanced-tracking.js`	769 B
`./dist/assets/js/googlesitekit-base.********************.js`	1.57 kB
`./dist/assets/js/googlesitekit-i18n.js`	3.92 kB
`./dist/assets/js/googlesitekit-idea-hub-post-list-notice.********************.js`	471 B
`./dist/assets/js/runtime.********************.js`	763 B

_{compressed-size-action}

danielgent · 2021-07-27T12:08:28Z

assets/js/modules/analytics/components/settings/SettingsView.js

@@ -42,6 +42,7 @@ export default function SettingsView() {
 	const isGA4Enabled = useFeature( 'ga4setup' );
 	const ga4PropertyID = useSelect( ( select ) => isGA4Enabled ? select( MODULES_ANALYTICS_4 ).getPropertyID() : '' );
 	const ga4MeasurementID = useSelect( ( select ) => isGA4Enabled ? select( MODULES_ANALYTICS_4 ).getMeasurementID() : '' );
+	const webDataStreamID = useSelect( ( select ) => isGA4Enabled ? select( MODULES_ANALYTICS_4 ).getWebDataStreamID() : '' );


I think this needs double checking. For me it was opening a web data stream for a totally different property (which makes no sense given the Url includes the correct property!)

danielgent · 2021-07-27T12:08:48Z

assets/js/modules/analytics/components/settings/SettingsView.js

+				<div className="googlesitekit-settings-module__meta-item">
+					<h5 className="googlesitekit-settings-module__meta-item-type">
+						&nbsp;
+					</h5>


Easiest and safest way to make the Links vertically aligned as per the AC (but this always feels a bit hacky!)

This needs hiding on mobile (as per updated AC)

We should make the links sibling elements of their respective DisplaySettings instead. We want the link to be clearly associated with the value and putting it in a separate element like this spaces it in the grid like a separate thing. This also has the potential to wrap on smaller screens where it would lose the positional context as to what it is referring to. Keeping them in the same parent also has the benefit of not requiring a hack to preserve the alignment 👍

Makes sense. Especially when going onto mobile

What do you think about the spacing? I just added one space.

It does look a little tight but also makes it more clear what the link refers to. I don't think we should get into adding  s or another class to add space as this seems a bit arbitrary and makes the link a bit more ambiguous.

What do you think about this?

This is how it looks if we also passed the small prop to the Links? I realize the ACs specifically say that it should be the same size but I think it might look a bit better with a reduced size and we have an existing prop to do this. Seeing the options side-by-side would also help make the case for changing that in case we feel it looks better 🙂 We'd have to ask about changing it but I think prefer the smaller link. What do you think?

Let's keep it as it is in the ACs for now, we can always follow-up with a super simple PR if we decide to tweak the Link size later.

I don't think there's anything actionable here for me now @aaemnnosttv ?

danielgent · 2021-07-27T12:09:17Z

assets/js/modules/analytics/components/settings/SettingsView.js

+					<Link
+						href={ editViewSettingsURL }
+						external
+						className="googlesitekit-settings-module__inline-link"


This seemed the safest way to override using CSS without making cascading changes through the whole site

Change of plan: remove this className and instead wrap the links with <p className="googlesitekit-settings-module__meta-item-data">

See my previous comment about placement. If needed, I think the inherit prop of the Link component should be all we need to match the surrounding text.

Much better. It works

assets/sass/components/settings/_googlesitekit-settings-module.scss

assets/js/modules/analytics/components/settings/SettingsView.js

aaemnnosttv

Thanks @danielgent – this looks good overall. I've left a few comments here as well as replies to some of your annotations. Let me know if you have any questions 👍

assets/js/modules/analytics/components/settings/SettingsView.js

aaemnnosttv · 2021-08-02T11:31:49Z

assets/js/modules/analytics/components/settings/SettingsView.js

+				<div className="googlesitekit-settings-module__meta-item">
+					<h5 className="googlesitekit-settings-module__meta-item-type">
+						&nbsp;
+					</h5>


We should make the links sibling elements of their respective DisplaySettings instead. We want the link to be clearly associated with the value and putting it in a separate element like this spaces it in the grid like a separate thing. This also has the potential to wrap on smaller screens where it would lose the positional context as to what it is referring to. Keeping them in the same parent also has the benefit of not requiring a hack to preserve the alignment 👍

aaemnnosttv · 2021-08-02T11:33:36Z

assets/js/modules/analytics/components/settings/SettingsView.js

+					<Link
+						href={ editViewSettingsURL }
+						external
+						className="googlesitekit-settings-module__inline-link"


See my previous comment about placement. If needed, I think the inherit prop of the Link component should be all we need to match the surrounding text.

assets/js/modules/analytics/components/settings/SettingsView.js

Conflicts: assets/js/modules/analytics/components/settings/SettingsView.js

assets/js/modules/analytics/components/settings/SettingsView.js

aaemnnosttv

Thanks @danielgent – this looks good as defined. See my question about the size of the link, but otherwise this looks good to go.

aaemnnosttv

Looks like I missed an important detail in the last review!

aaemnnosttv · 2021-08-03T07:12:16Z

assets/js/modules/analytics/components/settings/SettingsView.js

 								<DisplaySetting value={ ga4MeasurementID } />
 							</p>
 						</div>
+						<div className="googlesitekit-settings-module__meta-item">
+							<h5 className="googlesitekit-settings-module__meta-item-type">


Not sure how I missed this before, but this is duplicating the output for the Measurement ID

Fixed now!

That's kind of funny. It even has screenshots with the duplicate Measurement Id and we still didn't spot it 😛

Daniel Gent added 4 commits July 27, 2021 13:13

Move edit UA property link up and change text.

698adda

Add GA4 property link.

876482f

Add empty headers to vertically align.

cbb706d

Override font sizes.

3da07d8

google-cla bot added the cla: yes label Jul 27, 2021

Target CSS more specifically.

42dd713

danielgent mentioned this pull request Jul 27, 2021

Provide link in Analytics settings panel to update GA4 configuration and improve UI for related UA link #3702

Closed

Remove comment.

aa60b3a

danielgent commented Jul 27, 2021

View reviewed changes

danielgent commented Jul 29, 2021

View reviewed changes

assets/sass/components/settings/_googlesitekit-settings-module.scss Outdated Show resolved Hide resolved

danielgent commented Jul 29, 2021

View reviewed changes

assets/js/modules/analytics/components/settings/SettingsView.js Outdated Show resolved Hide resolved

aaemnnosttv requested changes Aug 2, 2021

View reviewed changes

Daniel Gent added 6 commits August 2, 2021 13:55

Merge branch 'develop'.

c0bbfe4

Conflicts: assets/js/modules/analytics/components/settings/SettingsView.js

Move new links to same line.

bb0dc9a

Remove CSS rule now.

8cb7ef3

Remove CSS styling and use inherit prop on Links.

eb52e72

Use escapeURI.

589a311

Rename to editDataStreamSettingsURL.

df0c62a

danielgent commented Aug 2, 2021

View reviewed changes

assets/js/modules/analytics/components/settings/SettingsView.js Outdated Show resolved Hide resolved

aaemnnosttv approved these changes Aug 2, 2021

View reviewed changes

aaemnnosttv requested changes Aug 3, 2021

View reviewed changes

Remove duplicate cell.

57ce76a

aaemnnosttv approved these changes Aug 4, 2021

View reviewed changes

aaemnnosttv merged commit 3d6252e into develop Aug 4, 2021

aaemnnosttv deleted the enhancment/3702-link-Analytics-update-GA4-UA-link branch August 4, 2021 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide link in Analytics settings panel to update GA4 configuration and improve UI for related UA link #3766

Provide link in Analytics settings panel to update GA4 configuration and improve UI for related UA link #3766

danielgent commented Jul 27, 2021 •

edited

Loading

github-actions bot commented Jul 27, 2021 •

edited

Loading

danielgent Jul 27, 2021

danielgent Jul 27, 2021

danielgent Jul 29, 2021

aaemnnosttv Aug 2, 2021

danielgent Aug 2, 2021

aaemnnosttv Aug 2, 2021

aaemnnosttv Aug 3, 2021

danielgent Aug 3, 2021

danielgent Jul 27, 2021

danielgent Jul 29, 2021

aaemnnosttv Aug 2, 2021

danielgent Aug 2, 2021

aaemnnosttv left a comment

aaemnnosttv Aug 2, 2021

aaemnnosttv Aug 2, 2021

aaemnnosttv left a comment

aaemnnosttv left a comment

aaemnnosttv Aug 3, 2021

danielgent Aug 3, 2021

Provide link in Analytics settings panel to update GA4 configuration and improve UI for related UA link #3766

Provide link in Analytics settings panel to update GA4 configuration and improve UI for related UA link #3766

Conversation

danielgent commented Jul 27, 2021 • edited Loading

Summary

Relevant technical choices

Checklist

github-actions bot commented Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielgent commented Jul 27, 2021 •

edited

Loading

github-actions bot commented Jul 27, 2021 •

edited

Loading