Add link to analytics options in submenu and make all settings sections linkable by johnwatkins0 · Pull Request #5216 · ampproject/amp-wp

johnwatkins0 · 2020-08-10T20:50:01Z

Summary

In #5155 we removed the Analytics menu page and placed the analytics option on the settings screen. This restores that menu item with a link to the Analytics section of the menu page.

Additional changes:

Passed the URL hash into the root settings screen component so that it can be used to scroll to sections after the UI loads
Moved the drawers on the settings screen up to the root settings screen component so that all the handling of IDs/anchors can be in one file
Created an AnalyticsOptionsSubmenu service class, which is a stripped-down version of the class that was removed in Add Analytics option to settings screen #5155. It inserts a link into the AMP admin menu in the second position.
Created a get_menu_slug method in the OptionsMenu class for consumption in AnalyticsOptionsSubmenu.

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).

…ns linkable

github-actions · 2020-08-11T15:34:59Z

Plugin builds for 249fb03 are ready 🛎️!

Download development build
Download production build

…190-settings-screen-anchor-links * 'develop' of github.com:ampproject/amp-wp: Revert "Replace astra with neve for testing since astra is suspended" Unset `$_SERVER['REQUEST_URI']` Fix lint error Default to localhost if `$_SERVER['HTTP_HOST']` is not set Remove check for WP CLI context Default parsed URL host to localhost if in WP CLI context Default parsed URL host to localhost if in WP CLI context Add null as return type Return null if the URL cannot be parsed when in a WP CLI environment Padding: adjust padding on mobile-width close button Ensure non-AMP link in paired browsing does not redirect to AMP version Update dependency @wordpress/data to v4.22.3 Update ReaderThemes::get_themes() to remove any themes that lack slug Also make name optional, falling back to slug Make theme description optional for Reader themes Make homepage a optional field for Reader themes Wizard: move close button to the top in mobile view Defer mobile redirection to end of template_redirect action Fix test referencing core theme Abort post-processing if an expected template was not rendered

westonruter · 2020-08-12T18:59:41Z

assets/src/settings-page/index.js

 * Settings page application root.
+ *
+ * @param {Object} props Component props.
+ * @param {string} props.section The initially focused section.


Maybe focusedSection would be more clear?

Done in e41c971

westonruter · 2020-08-12T19:01:38Z

assets/src/settings-page/index.js

+		const focusedSection = document.getElementById( section );
+
+		if ( focusedSection ) {
+			focusedSection.scrollIntoView();


Would smooth scrolling be desirable?

Suggested change

focusedSection.scrollIntoView();

focusedSection.scrollIntoView( { behavior: 'smooth' } );

Alternatively, for accessibility, I think what is needed is to actually focus() on the first focusable element.

So this should be looking for the first button or input that is inside the focusedSection and then focus() on it. This will automatically scroll and add the necessary keyboard focus.

Done in e41c971d9 with select elements also included

westonruter · 2020-08-12T19:12:45Z

src/Admin/AnalyticsOptionsSubmenu.php

+			[ $old_menu[0] ],
+			[
+				[
+					__( 'AMP Analytics Options', 'amp' ),


This needs to be shortened to just Analytics:

Done in b28aa90

westonruter · 2020-08-12T19:13:16Z

src/Admin/AnalyticsOptionsSubmenu.php

+			array_slice( $old_menu, 1 )
+		);
+
+		$submenu[ $this->parent_menu_slug ] = array_values( $new_menu ); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited


Should not $submenu be modified with add_submenu_page() rather than doing this and the above?

Ah, you're right. Updated in 0299a03. Didn't realize it would be so simple to link to the parent menu page with a hash appended.

westonruter · 2020-08-12T19:17:13Z

assets/src/settings-page/index.js

 			<ErrorContextProvider>
 				<Providers>
-					<Root optionsRestPath={ OPTIONS_REST_PATH } />
+					<Root section={ global.location.hash.replace( /^#/, '' ) } />


I think this can be improved… namely, if I am already on the AMP Settings screen and click the admin menu item, then I am jumped down the screen without the section expanding. Therefore, it would be preferable if there was also a hashchange event that would re-render this tree whenever it fires. This will ensure that the section always reflects what is currently in the location.hash.

I did this in 754aadd but it was surprisingly complicated to get it to work in all the scenarios (initial load of the page, changing the hash in the URL location bar, clicking the Analytics link in the menu), and I'm not sure how I feel about how much code I had to add. It's working, though.

…ithub.com/ampproject/amp-wp into feature/5190-settings-screen-anchor-links

…n linked section

westonruter · 2020-08-12T20:56:10Z

assets/src/settings-page/index.js

+ *
+ * @param {Object} props Component props.
+ * @param {string} props.focusedSection The initially focused focusedSection.


Suggested change

*

* @param {Object} props Component props.

* @param {string} props.focusedSection The initially focused focusedSection.

westonruter · 2020-08-12T20:56:43Z

assets/src/settings-page/index.js

+Root.propTypes = {
+	focusedSection: PropTypes.string,
+};


Suggested change

Root.propTypes = {

focusedSection: PropTypes.string,

};

assets/src/settings-page/index.js

pierlon · 2020-08-13T00:01:40Z

assets/src/settings-page/index.js

+				<h2 id="advanced-settings">
 					{ __( 'Advanced Settings', 'amp' ) }
 				</h2>
 				<MobileRedirection />


Why not put the ID for the section here instead of inside of the component?

Yeah, I like having all the IDs at this level. Updated in 5e7b5de

pierlon · 2020-08-13T00:03:36Z

assets/src/settings-page/index.js

+					id="supported-templates"
+					initialOpen={ 'supported-templates' === focusedSection }
+				>
+					<SupportedTemplates />


Same question as above.

In this case the ID is set just above on the AMPDrawer component.

pierlon · 2020-08-13T00:09:18Z

tests/e2e/utils/visit-admin-page-with-hash.js

+import { getPageError } from '@wordpress/e2e-test-utils';
+
+function createURLWithHash( WPPath, query = '', hash = '' ) {
+	const url = new URL( 'http://localhost:8890' );


This URL should be available via the environment variable WP_BASE_URL, so no need to hard code it.

Updated in 92ec1f3

pierlon · 2020-08-13T00:10:06Z

tests/e2e/utils/visit-admin-page-with-hash.js

+ * @param {string} query String to be serialized as query portion of URL.
+ * @param {string} hash URL hash.
+ */
+export async function visitAdminPageWithHash( adminPath, query, hash = null ) {


If defaulting hash to null is still wanted I'd recommend updating the docblock to include null, otherwise I'd recommend defaulting to an empty string:

Suggested change

export async function visitAdminPageWithHash( adminPath, query, hash = null ) {

export async function visitAdminPageWithHash( adminPath, query, hash = '' ) {

Updated in 92ec1f3

tests/php/src/Admin/AnalyticsOptionsSubmenuTest.php

…nent

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

johnwatkins0 · 2020-08-14T01:10:56Z

src/Admin/Polyfills.php

 	 * @param WP_Styles $wp_styles The WP_Styles instance for the current page.
 	 */
 	public function register_shimmed_styles( $wp_styles ) {
+		if ( version_compare( get_bloginfo( 'version' ), '5.4', '<' ) ) {


Unrelated change: Noticed some undesirable CSS effects when working in 5.2 locally, so this will force the polyfilled wp-components styles. Note this only runs on AMP screens.

google-cla · 2020-08-14T03:14:05Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

pierlon · 2020-08-14T03:16:02Z

@googlebot I consent.

pierlon · 2020-08-14T03:16:12Z

Some tests were modifying the global $submenu without restoring it's original value, so I've fixed that in 70e517b. I've also updated OptionsMenuTest::test_add_menu_items to perform the correct assertions.

pierlon · 2020-08-14T03:45:36Z

On a larger viewport the scroll to the Analytics section doesn't seem to have much have an effect:

johnwatkins0 · 2020-08-14T05:54:37Z

On a larger viewport the scroll to the Analytics section doesn't seem to have much have an effect:

That's true, but it's just because the analytics section is last on the page so it can't scroll any further than that. I suppose the workarounds would be to move that section up or add a bit of extra padding to the bottom of the page.

pierlon · 2020-08-14T06:03:00Z

Could we focus the drawer when selected by hash? Similar to how GitHub does it, where if I link to a comment it adds a border around it:

westonruter · 2020-08-14T06:38:08Z

Could we focus the drawer when selected by hash? Similar to how GitHub does it, where if I link to a comment it adds a border around it:

I'd say that would be an optional enhancement. GitHub styles this via:

:target .timeline-comment {
    border-color: #2188ff;
    box-shadow: 0 0 0 0.2em #c8e1ff;
}

However, the drawers depend on box-shadow to achieve their effect. If we do any such highlighting, I'd say perhaps we go with an initial faint yellow that fades out (e.g. via a CSS animation).

But yeah, I'd say this is not really necessary. After all, the first focusable element will have the outline. I suppose the the alternative would be remove the :not(.components-panel__body-toggle) so that the drawer button gets the border which would have a similar effect to what GitHub is doing:

--- a/assets/src/settings-page/index.js
+++ b/assets/src/settings-page/index.js
@@ -105,7 +105,7 @@ function scrollFocusedSectionIntoView( focusedSectionId ) {
 		return;
 	}
 
-	const firstInput = focusedSectionElement.querySelector( 'input, select, textarea, button:not(.components-panel__body-toggle)' );
+	const firstInput = focusedSectionElement.querySelector( 'input, select, textarea, button' );
 
 	if ( firstInput ) {
 		firstInput.focus();

westonruter · 2020-08-14T06:52:24Z

I noticed that when going to /wp-admin/admin.php?page=amp-options#reader-themes-drawer it doesn't cause the Reader theme drawer to open initially.

johnwatkins0 · 2020-08-14T14:14:23Z

a856c71 adds scrolling to the reader themes drawer if it's on the page (note I changed that HTML ID from reader-themes-drawer to reader-themes). I also removed that :not(.components-panel__body-toggle) in the selector, so the toggle button will always be focused for these drawers.

Another thing, I noticed that element.scrollIntoView({behavior: smooth}) seemed to work much more reliably than focusElement.focus({behavior: smooth}) so I updated it to use scrollIntoView on the main element and then focus the focus element without smooth scroll.

westonruter · 2020-08-14T17:48:12Z

focusElement.focus({behavior: smooth})

Oh, I didn't even the focus() method as supporting {behavior:smooth}: https://developer.mozilla.org/en-US/docs/Web/API/HTMLOrForeignElement/focus

So, good to switch to scrollIntoView 👍

westonruter · 2020-08-14T17:51:50Z

assets/src/settings-page/style.css

 }

-#reader-themes-drawer {
+#reader-themes {


Thanks for making this change. I was going to suggest it but I didn't want to nit pick… too much. 😊

johnwatkins0 · 2020-08-14T18:05:11Z

Oh, I didn't even the focus() method as supporting {behavior:smooth}:

Huh, you're right. I could've sworn I learned this was a thing recently, but I'm no longer able to find it 🤷‍♂️ No wonder it didn't work.

westonruter · 2020-08-14T18:12:27Z

For some reason deeplinking to #reader-themes is not causing scrolling to occur. If I try navigating in a new tab to #analytics I get scrolled down, but not if I go to #reader-themes. If, however, I put the scroll logic in a 100ms timeout then the Reader Themes will scroll into view. It's strange because the Reader Themes section does exist in the DOM and it is being rendered. Not a blocker since this section is near the top anyway and it is still getting expanded, but it is still a mystery.

…avior:smooth CSS present

Add link to analytics options in submenu and make all settings sectio…

8c819bc

…ns linkable

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

johnwatkins0 added 8 commits August 10, 2020 17:19

Fix tests for options menu

d159ec2

PHPUnit test: adjust expected menu item count

52e0327

PHPUnit: restore expected link for user with dev tools on

24033f2

Add e2e test for settings page anchor linking

145337e

Remove unused imports

402eb2d

Actually change dev tools setting in phpunit test

00c18fd

Remove unnecessary use statement

004075c

Remove unnecessary space

e4f75d6

johnwatkins0 marked this pull request as ready for review August 11, 2020 15:31

johnwatkins0 requested review from pierlon, schlessera and westonruter August 11, 2020 15:31

Comment fixes

ba960bd

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

westonruter requested changes Aug 12, 2020

View reviewed changes

johnwatkins0 added 4 commits August 12, 2020 14:37

Add menu item with add_submnu_page

0299a03

Merge branch 'feature/5190-settings-screen-anchor-links' of https://g…

987a3e9

…ithub.com/ampproject/amp-wp into feature/5190-settings-screen-anchor-links

Shorten Analytics menu link name

b28aa90

Settings anchor linking: smoothly scroll to first focusable element i…

e41c971

…n linked section

westonruter reviewed Aug 12, 2020

View reviewed changes

johnwatkins0 added 3 commits August 12, 2020 16:11

Update handling of scroll and hash behavior on settings screen

754aadd

Update how drawer initialOpen resets (fixing Jest test)

5827c88

Resolve small failures in PHPUnit tests

999609f

pierlon suggested changes Aug 13, 2020

View reviewed changes

johnwatkins0 and others added 2 commits August 12, 2020 19:59

Settings screen: move mobile redirection html ID up to the root compo…

5e7b5de

…nent

PHPUnit docblock improvements

a7c1459

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

johnwatkins0 added 2 commits August 13, 2020 20:05

Polyfill wp-components CSS on Amp screens if wp version is below 5.4

a202f60

Remove carousel button right margin

4ed39e2

johnwatkins0 commented Aug 14, 2020

View reviewed changes

Restore the global admin menu when done testing

70e517b

google-cla bot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Aug 14, 2020

google-cla bot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Aug 14, 2020

Modifications to section scrollTo behavior on settings screen

a856c71

westonruter reviewed Aug 14, 2020

View reviewed changes

westonruter added 2 commits August 14, 2020 11:13

Skip gettingElementById for empty strings

3302d9c

Remove redundant behavior:smooth from scrollIntoView since scroll-beh…

249fb03

…avior:smooth CSS present

westonruter approved these changes Aug 14, 2020

View reviewed changes

pierlon approved these changes Aug 14, 2020

View reviewed changes

westonruter added this to the v2.0 milestone Aug 14, 2020

westonruter merged commit b246864 into develop Aug 14, 2020

westonruter deleted the feature/5190-settings-screen-anchor-links branch August 14, 2020 19:46

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

	focusedSection.scrollIntoView();
	focusedSection.scrollIntoView( { behavior: 'smooth' } );

	*
	* @param {Object} props Component props.
	* @param {string} props.focusedSection The initially focused focusedSection.

	export async function visitAdminPageWithHash( adminPath, query, hash = null ) {
	export async function visitAdminPageWithHash( adminPath, query, hash = '' ) {

Conversation

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

Uh oh!

Summary

Checklist

Uh oh!

github-actions bot commented Aug 11, 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!

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!

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!

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

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-cla bot commented Aug 14, 2020

Uh oh!

pierlon commented Aug 14, 2020

Uh oh!

pierlon commented Aug 14, 2020

Uh oh!

pierlon commented Aug 14, 2020

Uh oh!

johnwatkins0 commented Aug 14, 2020

Uh oh!

pierlon commented Aug 14, 2020

Uh oh!

westonruter commented Aug 14, 2020

Uh oh!

westonruter commented Aug 14, 2020

Uh oh!

johnwatkins0 commented Aug 14, 2020

Uh oh!

westonruter commented Aug 14, 2020

johnwatkins0 commented Aug 10, 2020 •

edited

Loading

github-actions bot commented Aug 11, 2020 •

edited

Loading

pierlon Aug 13, 2020 •

edited

Loading