Enhance/6261 key metrics settings toggle #6718

kuasha420 · 2023-03-13T18:29:36Z

Summary

Addresses issue:

Implement settings toggle to show/hide the Key metrics widget #6261

Relevant technical choices

PR Author 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 5.2 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/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

jimmymadon

Thanks for all the refactoring @kuasha420! I've had a look at the non-test files in this PR and added some minor comments. Overall looks great.

assets/js/components/settings/SettingsKeyMetrics.js

assets/js/googlesitekit/datastore/user/key-metrics.js

jimmymadon · 2023-03-17T02:02:31Z

includes/Core/Key_Metrics/Key_Metrics_Settings.php

-		return array();
+		return array(
+			'widgetSlugs'    => array(),
+			'isWidgetHidden' => true,


I think this value is storing whether the widget is "hidden" or not, once it is setup. So we can default this to false perhaps? Semantically, if the widget is not set up, then it is technically "NOT" hidden even though it is invisible. It is just not available because there aren't any widgets to be displayed. If there are no key-metrics returned, then the widget area will automatically be invisible even though the isWidgetHidden flag is set to false.

kuasha420

This is the PR where @jimmymadon and I decided to refactor a bunch of stuff to make things more streamlined. So leaving some comments to provide some context to potential code reviewers.

kuasha420 · 2023-03-20T11:54:38Z

assets/js/components/settings/SettingsAdmin.js

-					{ isUserInputCompleted && (
-						<Layout
-							title={ __( 'Key metrics', 'google-site-kit' ) }
-							header
-							rounded
-						>
-							<div className="googlesitekit-settings-module googlesitekit-settings-module--active googlesitekit-settings-user-input">
-								<Grid>
+					<Layout
+						title={ __( 'Key metrics', 'google-site-kit' ) }
+						header
+						rounded
+					>
+						<div className="googlesitekit-settings-module googlesitekit-settings-module--active googlesitekit-settings-user-input">
+							<Grid>
+								<SettingsKeyMetrics />
+							</Grid>
+							<Grid>
+								{ isUserInputCompleted && (
 									<UserInputPreview
 										goTo={ goTo }
 										noHeader
 										noFooter
 										settingsView
 										showIndividualCTAs
 									/>
-								</Grid>
-							</div>
-						</Layout>
-					) }
-
-					{ isUserInputCompleted === false && (
-						<UserInputSettings isDismissible={ false } rounded />
-					) }
+								) }
+								{ isUserInputCompleted === false && (
+									<div className="googlesitekit-user-input__notification">
+										<Cell>
+											<p>
+												<span>
+													{ __(
+														'Answer 3 quick questions to help us show the most relevant data for your site',
+														'google-site-kit'
+													) }
+												</span>
+											</p>
+											<Link href={ userInputURL }>
+												{ __(
+													'Personalize your metrics',
+													'google-site-kit'
+												) }
+											</Link>
+										</Cell>
+									</div>
+								) }
+							</Grid>
+						</div>
+					</Layout>


This changes are necessary due to the following reason:

The user should be able disable the Key Metrics Widget as long as the getKeyMetrics selector from CORE_WIDGETS returns one or more widgets, be it based on User Input Question or Manually Picked.

However, As long as user has not completed the user input questions, they'll continue to get the CTA to answer them. This is regardless of whether user has manually selected key metrics widgets or turned off the Key Metrics using the toggle.

Therefore, as long as userInput feature flag is enabled, the Key Metrics section in settings will remain and will be potentially populated with SettingsKeyMetrics and User Input CTA or Preview.

The UserInputSettings could no longer be used because it would cause multiple Key Metrics title/sections. Therefore I used the Link and p and styled them accordingly. Moreover, the UserInputSettings component will be removed once the user input dashboard CTA is created according to design in #6210.

assets/js/googlesitekit/datastore/user/key-metrics.js

assets/js/modules/analytics-4/components/widgets/EngagedTrafficSourceWidget.js

assets/js/modules/analytics-4/components/widgets/EngagedTrafficSourceWidget.test.js

assets/js/util/key-metrics.js

jimmymadon

@kuasha420 Thanks for these - I've added one comment if you want to move the key-metrics.js within utlils to the /components folder. But this is not a big issue and we don't have a definite pattern here - so I will leave this up to you. But everything else looks good to me.

eugene-manuilov

Thanks, @kuasha420. This is a good start, but there are things that we need address before merging this PR. Please, take a look at my comments.

assets/js/googlesitekit/datastore/user/key-metrics.js

assets/js/googlesitekit/widgets/datastore/constants.js

assets/js/googlesitekit/widgets/datastore/key-metrics.js

assets/js/modules/analytics-4/components/widgets/EngagedTrafficSourceWidget.js

assets/js/util/key-metrics.js

assets/js/components/settings/SettingsKeyMetrics.js

assets/js/components/settings/SettingsAdmin.js

github-actions · 2023-05-30T13:21:43Z

Build files for f1a254d have been deleted.

eugene-manuilov

Thanks, @kuasha420. Almost looks good to me. Left two more comments.

assets/js/components/settings/SettingsKeyMetrics.js

assets/js/components/settings/SettingsAdmin.js

kuasha420 · 2023-05-30T21:53:13Z

@eugene-manuilov I've added more context on the changes. Also the VRT issue was fixed after merging develop. Please, let me know what you think. Cheers!

eugene-manuilov

@kuasha420, could you please fix merge conflicts?

hussain-t · 2023-05-31T16:08:59Z

@kuasha420 @eugene-manuilov, I think there is confusion regarding #6210. The feature description clearly says:

The Key metrics Setup CTA placeholder component created in #6209 should be designed fully in this issue.

KeyMetricsSetupCTAWidget was the only component added in #6209. So, as part of 6210, the KeyMetricsSetupCTAWidget component should be designed, not UserInputSettings. cc: @jimmymadon

jimmymadon · 2023-06-01T11:06:38Z

@hussain-t Originally, the UserInputSettings component contained the "KeyMetrics Setup CTA Banner" to originally ask users to answer the questionnaire. This component was being used in two places, the Dashboard and the Admin Settings panel. This was implemented as a BannerNotification based on the requirements at that time as part of User Input V1 epic. So essentially, all we have to do is to "refactor" (simply delete) the old UserInputSettings component and design the KeyMetricsSetupCTAWidget as per the ACs of #6210.

kuasha420 · 2023-06-01T12:03:21Z

@eugene-manuilov all sorted. Please have a look. Cheers!

hussain-t · 2023-06-01T12:32:37Z

@hussain-t Originally, the UserInputSettings component contained the "KeyMetrics Setup CTA Banner" to originally ask users to answer the questionnaire. This component was being used in two places, the Dashboard and the Admin Settings panel. This was implemented as a BannerNotification based on the requirements at that time as part of User Input V1 epic. So essentially, all we have to do is to "refactor" (simply delete) the old UserInputSettings component and design the KeyMetricsSetupCTAWidget as per the ACs of #6210.

Thanks for the clarification, @jimmymadon!

eugene-manuilov

Thanks, @kuasha420. LGTM!

kuasha420 added 15 commits March 3, 2023 17:40

Add SettingsKeyMetrics component.

6360116

Merge branch 'develop' into enhance/6261-key-metrics-settings-toggle.

91fdbc2

Merge branch 'develop' into enhance/6261-key-metrics-settings-toggle.

4647c7d

Cleanup incorrect implementation logic.

cb05d5b

Update Key Metrics section of settings page.

b19b7f3

Merge branch 'develop' into enhance/6261-key-metrics-settings-toggle.

e6afe31

Update key metrics related selectors and actions.

2951521

Update fetch store actions.

bf56f74

Update SettingsKeyMetrics component.

9511ea5

Update key-metrics datastore tests.

0f74f2b

Add loading check to SettingsKeyMetrics.

3d60c0d

Merge branch 'develop' into enhance/6261-key-metrics-settings-toggle.

792a891

Add default value to Key Metrics Settings.

a534afb

Update test.

3b1c139

Remove unnecessary return value from resolver.

c18a180

This comment was marked as resolved.

Sign in to view

kuasha420 added 3 commits March 14, 2023 01:28

Add class to user input section.

c05b1ea

Don't render key matrics toggle if there is no key metrics.

c3db905

Fix incorrect stores.

cbb4567

jimmymadon requested changes Mar 17, 2023

View reviewed changes

kuasha420 added 8 commits March 17, 2023 17:32

Fix typos.

44bf9b7

Change the isWidgetHidden default value.

bf0e78a

Add isKeyMetricsWidgetHidden selector.

741c1b7

Add tests for isKeyMetricsWidgetHidden.

af5573d

Refactor SettingsKeyMetrics to use isKeyMetricsWidgetHidden.

3953f3b

Add keyMetricsWidgetHidden check to key metrics widgets.

d0fc19f

Update tests for key metrics widgets.

945b21c

Fix typos and mistakes.

67846be

kuasha420 commented Mar 20, 2023

View reviewed changes

nfmohit mentioned this pull request Apr 13, 2023

Do not auto-select matched UA property in GA setup flow #6820

Closed

kuasha420 added 2 commits May 18, 2023 19:29

Update tests.

2377a22

Move the Grid to SettingsKeyMetrics.

2009950

jimmymadon approved these changes May 22, 2023

View reviewed changes

eugene-manuilov requested changes May 23, 2023

View reviewed changes

kuasha420 added 8 commits May 30, 2023 14:49

Merge branch 'develop' into enhance/6261-key-metrics-settings-toggle.

9be7349

Rename selector.

01f5f73

Move key metrics related infra from widgets datastore.

3de5d22

Add prop types to key metrics widgets.

fa316fd

Refactor KM widget tests to use provideKeyMetrics.

f9d21e2

Add row and cell.

857e4c3

Update SettingsAdmin before revert.

ee1ece8

Move regex to key-metrics.

a2515f5

eugene-manuilov requested changes May 30, 2023

View reviewed changes

assets/js/components/settings/SettingsKeyMetrics.js Show resolved Hide resolved

assets/js/components/settings/SettingsAdmin.js Outdated Show resolved Hide resolved

Merge branch 'develop' into enhance/6261-key-metrics-settings-toggle.

cc40ddd

kuasha420 mentioned this pull request May 31, 2023

Design Key metrics Setup CTA #6210

Closed

eugene-manuilov requested changes May 31, 2023

View reviewed changes

kuasha420 added 2 commits June 1, 2023 16:14

Merge branch 'develop' into enhance/6261-key-metrics-settings-toggle.

b65acf4

Remove KM widget test files.

bbc6bd3

Fix VRT.

f1a254d

eugene-manuilov approved these changes Jun 2, 2023

View reviewed changes

eugene-manuilov merged commit 5f79878 into develop Jun 2, 2023

eugene-manuilov deleted the enhance/6261-key-metrics-settings-toggle branch June 2, 2023 09:57

hussain-t mentioned this pull request Jun 5, 2023

Enhance/#6210 - Design Key metrics Setup CTA #7127

Merged

18 tasks

techanvil mentioned this pull request Jul 6, 2023

Check for gathering data state in Search Console and Analytics before showing User Input notification. #7198

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance/6261 key metrics settings toggle #6718

Enhance/6261 key metrics settings toggle #6718

kuasha420 commented Mar 13, 2023 •

edited

Loading

This comment was marked as resolved.

jimmymadon left a comment

jimmymadon Mar 17, 2023

kuasha420 left a comment

kuasha420 Mar 20, 2023

jimmymadon left a comment

eugene-manuilov left a comment

github-actions bot commented May 30, 2023 •

edited

Loading

eugene-manuilov left a comment

kuasha420 commented May 30, 2023

eugene-manuilov left a comment

hussain-t commented May 31, 2023

jimmymadon commented Jun 1, 2023

kuasha420 commented Jun 1, 2023

hussain-t commented Jun 1, 2023

eugene-manuilov left a comment

Enhance/6261 key metrics settings toggle #6718

Enhance/6261 key metrics settings toggle #6718

Conversation

kuasha420 commented Mar 13, 2023 • edited Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

This comment was marked as resolved.

jimmymadon left a comment

Choose a reason for hiding this comment

jimmymadon Mar 17, 2023

Choose a reason for hiding this comment

kuasha420 left a comment

Choose a reason for hiding this comment

kuasha420 Mar 20, 2023

Choose a reason for hiding this comment

jimmymadon left a comment

Choose a reason for hiding this comment

eugene-manuilov left a comment

Choose a reason for hiding this comment

github-actions bot commented May 30, 2023 • edited Loading

eugene-manuilov left a comment

Choose a reason for hiding this comment

kuasha420 commented May 30, 2023

eugene-manuilov left a comment

Choose a reason for hiding this comment

hussain-t commented May 31, 2023

jimmymadon commented Jun 1, 2023

kuasha420 commented Jun 1, 2023

hussain-t commented Jun 1, 2023

eugene-manuilov left a comment

Choose a reason for hiding this comment

kuasha420 commented Mar 13, 2023 •

edited

Loading

github-actions bot commented May 30, 2023 •

edited

Loading