Add Analytics option to settings screen by johnwatkins0 · Pull Request #5155 · ampproject/amp-wp

johnwatkins0 · 2020-08-03T18:42:27Z

Summary

The menu page for AMP Analytics has been removed in favor of this new drawer on the AMP settings page:

The migration to the new way of handling options was straightforward, except I change how the analytics option was saved. **Previously: ** calling AMP_Options_Manager::update_option with the analytics key would add the new value without removing existing values. Entries were only deleted if they were passed in with a delete flag. **Now: ** I didn't see any reason to preserve that behavior. The React state on the settings screen receives the full array of analytics entries and passes back the modified array.

There may be a few unrelated CSS changes included in this PR; while working on the mobile view of this feature I saw some other parts of the settings mobile view in need of polish.

This also addresses an issue raised in support topic where in-house analytics do not need a type attribute):

Remove required attribute from Type input field and server-side validation.
Omit type from amp-analytics tag when Type is empty.
Remove the unnecessary readonly ID input field from the form entries.
Use UUID to generate unique IDs rather than hashing type and contents.
Stop needlessly showing ID in entry heading on admin screen, and use ordered list instead to differentiate entries.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

github-actions · 2020-08-03T22:01:49Z

Plugin builds for b5b6488 are ready 🛎️!

Download development build
Download production build

johnwatkins0 · 2020-08-03T22:12:12Z

includes/options/class-amp-options-manager.php

-	 * @param string $type    Optional. Message type, controls HTML class. Possible values include 'error',
-	 *                        'success', 'warning', 'info'. Default 'error'.
-	 */
-	private static function add_settings_error( $setting, $code, $message, $type = 'error' ) {


See #5013 (comment)

…analytics-to-settings-screen

assets/src/components/theme-card/index.js

…l in theme card

assets/src/settings-page/analytics.js

pierlon · 2020-08-05T00:40:23Z

assets/src/settings-page/analytics.js

+	return (
+		<div>
+
+			<details open={ ! Boolean( Object.keys( originalOptions.analytics ).length ) }>


Suggested change

<details open={ ! Boolean( Object.keys( originalOptions.analytics ).length ) }>

<details open={ detailsInitialOpen }>

Actually it looks like the state wasn't needed. Fixed in e5380c9

pierlon · 2020-08-05T02:04:09Z

assets/src/settings-page/analytics.js

+			<Button
+				isLink
+				onClick={ onDelete }
+				className="amp-analytics__delete-button"


I'd recommend making this button red instead of blue on hover.

Done in da8ea68

pierlon · 2020-08-05T02:07:24Z

assets/src/settings-page/analytics.js

+		<div>
+
+			<details open={ ! Boolean( Object.keys( originalOptions.analytics ).length ) }>
+				<summary>


Cursor should be a pointer when over the summary element.

Done in e5380c9

pierlon · 2020-08-05T02:10:55Z

assets/src/settings-page/analytics.js

+						label={ __( 'ID:', 'amp' ) }
+						type="text"
+						value={ isExistingEntry ? entryId : '' }
+						readOnly


I'd recommend adding a tooltip for this input to inform the user that the ID will be generated once the settings are saved.

Added help text to the two inputs in 076d206

We don't really even need the ID. It really doesn't serve any purpose as far as I can tell. I started to remove it in #4802.

Remove the unnecessary readonly ID input field from the form entries.

Ah, thanks for pointing that out. I was staying too faithful to the source.

pierlon · 2020-08-05T02:13:13Z

assets/src/settings-page/analytics.js

+					label={ __( 'JSON Configuration:', 'amp' ) }
+				>
+					<textarea
+						rows="10"


The "Save Changes" button should be disabled if the JSON entered is invalid.

I entertained this possibility, but for the fields in this section we're using built-in browser form validation, which at this point will do a better job of directing the user to the invalid fields.

I've thought about creating a validation context provider that could collect issues with the form along with their associated elements, and scroll the user to form issues when they try to submit. This would allow us to implement form validation behavior for things like the the mode selection options, which are not standard form elements. I might create an issue for that.

tests/e2e/specs/admin/analytics-options.js

…ediately returning Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

…s://github.com/ampproject/amp-wp into feature/5013-move-analytics-to-settings-screen

pierlon · 2020-08-06T05:02:12Z

assets/src/settings-page/analytics.js

+
+		const firstInput = inputWrapper.current.querySelector( 'input' );
+		if ( firstInput ) {
+			firstInput.focus();


This does not seem to work as intended when the drawer is first opened; the last input is focused instead of the first.

Yes, I just saw the same. While I appreciate the intention, I don't think it's necessary in this context. Removed in 877f1f8.

…nfig

westonruter · 2020-08-06T06:11:51Z

I'm amending the PR with the changes from #4802.

tests/php/test-amp-analytics-options.php

amedina · 2020-08-06T15:52:21Z

This looks great!

One nit pick: I would like to consider a better label for Analytics 1.

Testing now on local install.

amedina

This is great. Addressed comment offline.

westonruter · 2020-08-06T17:05:58Z

Perhaps we should retain the Analytics admin menu item and have it go to the settings screen with the Analytics section pre-expanded.

westonruter · 2020-08-06T17:33:17Z

Perhaps we should retain the Analytics admin menu item and have it go to the settings screen with the Analytics section pre-expanded.

Filed in #5190.

Changes addressed in #5155 (comment)

johnwatkins0 added 2 commits August 3, 2020 09:58

Add analytics to options REST endpoint

3f88167

Add Analytics to settings screen

f45669f

google-cla bot added the cla: yes Signed the Google CLA label Aug 3, 2020

johnwatkins0 added 7 commits August 3, 2020 14:40

Add e2e tests for analytics settings

160f75e

Update analytics-related tests

5989c7c

Comment updates

461c2ed

Remove whitespace at end of line

58dd10b

Fix undefined entry slug

2c17cbe

Analytics option PHPUnit test fixes

b7a0752

Fix analytics insert two options test

68ffeae

johnwatkins0 requested review from pierlon, schlessera and westonruter August 3, 2020 21:58

johnwatkins0 marked this pull request as ready for review August 3, 2020 21:58

johnwatkins0 self-assigned this Aug 3, 2020

Remove AMP_Options_Manager::add_settings_error

7cf0b25

johnwatkins0 commented Aug 3, 2020

View reviewed changes

johnwatkins0 added 2 commits August 3, 2020 21:07

Fix theme card heading styles

f62fc80

Merge remote-tracking branch 'origin/develop' into feature/5013-move-…

396681f

…analytics-to-settings-screen

schlessera previously requested changes Aug 4, 2020

View reviewed changes

assets/src/components/theme-card/index.js Outdated Show resolved Hide resolved

Remove heading classes and update e2e test for different heading leve…

c83d149

…l in theme card

pierlon suggested changes Aug 5, 2020

View reviewed changes

johnwatkins0 and others added 7 commits August 5, 2020 08:34

Return useCallback result instead of storing it in a variable and imm…

5dcf3cb

…ediately returning Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

Install @wordpress/icons

6f3aef7

Fix editor autosuggest weirdness.

1ad33b8

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

Replace sprintf with template literal

7946ddc

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

Add help text to analytics text inputs

076d206

Merge branch 'feature/5013-move-analytics-to-settings-screen' of http…

be041c0

…s://github.com/ampproject/amp-wp into feature/5013-move-analytics-to-settings-screen

Analytics: confirm remove entry and make button hover red

da8ea68

johnwatkins0 requested a review from westonruter August 6, 2020 04:32

pierlon reviewed Aug 6, 2020

View reviewed changes

westonruter added 3 commits August 5, 2020 22:15

Remove focus on type input since occurs when opening drawer

877f1f8

Remove unnecessaryrequirement that analytics have a type

70f9baf

Remove unnecessary deduplication of entries; test bad/missing JSON co…

e316faf

…nfig

westonruter added 3 commits August 5, 2020 23:30

Remove obsolete test for get_settings_errors

edf08f7

Add sanitization of ID for good measure

8fae32c

Restore test_two_analytics_components_added

42478f3

westonruter approved these changes Aug 6, 2020

View reviewed changes

westonruter requested review from amedina, pierlon and schlessera August 6, 2020 06:47

pierlon suggested changes Aug 6, 2020

View reviewed changes

tests/php/test-amp-analytics-options.php Outdated Show resolved Hide resolved

Fix grammar in comment

b586f1d

pierlon approved these changes Aug 6, 2020

View reviewed changes

amedina approved these changes Aug 6, 2020

View reviewed changes

Update analytics entry heading

7c2282d

westonruter mentioned this pull request Aug 6, 2020

Restore Analytics admin menu item to link to jump to expanded Analytics drawer on settings screen #5190

Closed

Warn when removing a non-empty config, not a non-empty type

b5b6488

westonruter merged commit d20c516 into develop Aug 6, 2020

westonruter deleted the feature/5013-move-analytics-to-settings-screen branch August 6, 2020 17:51

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Aug 8, 2020

johnwatkins0 mentioned this pull request Aug 11, 2020

Add link to analytics options in submenu and make all settings sections linkable #5216

Merged

3 tasks

johnwatkins0 added the WS:UX Work stream for UX/Front-end label Aug 12, 2020

pierlon mentioned this pull request Aug 30, 2020

Remove requirement for analytics to entries to have type supplied #4802

Closed

3 tasks

	<details open={ ! Boolean( Object.keys( originalOptions.analytics ).length ) }>
	<details open={ detailsInitialOpen }>

Conversation

johnwatkins0 commented Aug 3, 2020 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

github-actions bot commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Aug 6, 2020

Uh oh!

Uh oh!

amedina commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amedina left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter commented Aug 6, 2020

Uh oh!

westonruter commented Aug 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

johnwatkins0 commented Aug 3, 2020 •

edited by westonruter

Loading

github-actions bot commented Aug 3, 2020 •

edited

Loading

amedina commented Aug 6, 2020 •

edited

Loading