From 99d17aa504bd13ea00ed816b34c7d9eac5850a28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Thu, 30 Sep 2021 15:13:43 +0200 Subject: [PATCH 1/2] Remove subpath URLQueryParam from settings pages. Use WC navigation features instead. --- js/src/index.js | 2 +- js/src/settings/index.js | 15 ++++++++--- js/src/utils/urls.js | 18 +++---------- .../Controllers/Google/AccountController.php | 2 +- src/Menu/Settings.php | 27 +++++++++++++++++++ 5 files changed, 43 insertions(+), 21 deletions(-) diff --git a/js/src/index.js b/js/src/index.js index bfa8ad6f3a..5e82c8ec2d 100644 --- a/js/src/index.js +++ b/js/src/index.js @@ -113,7 +113,7 @@ addFilter( __( 'Settings', 'google-listings-and-ads' ), ], container: Settings, - path: '/google/settings', + path: '/google/settings/:subpath?', wpOpenMenu: 'toplevel_page_woocommerce-marketing', navArgs: { id: 'google-settings', diff --git a/js/src/settings/index.js b/js/src/settings/index.js index 57115bf77b..2e5a42b448 100644 --- a/js/src/settings/index.js +++ b/js/src/settings/index.js @@ -2,7 +2,7 @@ * External dependencies */ import { useEffect } from '@wordpress/element'; -import { getQuery, getHistory } from '@woocommerce/navigation'; +import { getHistory } from '@woocommerce/navigation'; /** * Internal dependencies @@ -17,8 +17,15 @@ import EditStoreAddress from './edit-store-address'; import EditPhoneNumber from './edit-phone-number'; import './index.scss'; -const Settings = () => { - const { subpath } = getQuery(); +/** + * Settings page component. + * + * @param {Object} [props] React props + * @param {Object} [props.params = {}] `path` params extracted by React ``. + * @param {string} [props.params.subpath] Sub path of a nested settings page. + * @return {JSX.Element} Settings page component. + */ +const Settings = ( { params: { subpath } = {} } ) => { const { google } = useGoogleAccount(); const isReconnectAccountsPage = subpath === subpaths.reconnectAccounts; @@ -31,7 +38,7 @@ const Settings = () => { }, [ isReconnectAccountsPage, google ] ); // Navigate to subpath is any. - switch ( subpath ) { + switch ( '/' + subpath ) { case subpaths.reconnectAccounts: return ; case subpaths.editPhoneNumber: diff --git a/js/src/utils/urls.js b/js/src/utils/urls.js index 927207c94b..cfa3224a42 100644 --- a/js/src/utils/urls.js +++ b/js/src/utils/urls.js @@ -39,24 +39,12 @@ export const getSettingsUrl = () => { }; export const getEditPhoneNumberUrl = () => { - return getNewPath( - { subpath: subpaths.editPhoneNumber }, - settingsPath, - null - ); + return getNewPath( null, settingsPath + subpaths.editPhoneNumber, null ); }; export const getEditStoreAddressUrl = () => { - return getNewPath( - { subpath: subpaths.editStoreAddress }, - settingsPath, - null - ); + return getNewPath( null, settingsPath + subpaths.editStoreAddress, null ); }; export const getReconnectAccountsUrl = () => { - return getNewPath( - { subpath: subpaths.reconnectAccounts }, - settingsPath, - null - ); + return getNewPath( null, settingsPath + subpaths.reconnectAccounts, null ); }; diff --git a/src/API/Site/Controllers/Google/AccountController.php b/src/API/Site/Controllers/Google/AccountController.php index 43a83ee0f8..508c7ec12b 100644 --- a/src/API/Site/Controllers/Google/AccountController.php +++ b/src/API/Site/Controllers/Google/AccountController.php @@ -88,7 +88,7 @@ protected function get_connect_callback(): callable { return function( Request $request ) { try { $next = $request->get_param( 'next' ); - $path = $next === 'setup-mc' ? '/google/setup-mc' : '/google/settings&subpath=/reconnect-accounts'; + $path = $next === 'setup-mc' ? '/google/setup-mc' : '/google/settings/reconnect-accounts'; return [ 'url' => $this->connection->connect( admin_url( "admin.php?page=wc-admin&path={$path}" ) ), diff --git a/src/Menu/Settings.php b/src/Menu/Settings.php index 1ec38fb248..72d47e0973 100644 --- a/src/Menu/Settings.php +++ b/src/Menu/Settings.php @@ -32,6 +32,33 @@ function() { ], ] ); + // TODO: Check if we can make it a bit DRYier, and reduce the below, to smth like `'path' => '/google/settings/:subpage?'` + // So far it's hard to guess the type of register_page options, + // which is documented to be the type of "Options for PageController::register_page()." ➰ + wc_admin_register_page( + [ + 'title' => __( 'Settings', 'google-listings-and-ads' ), + 'parent' => 'google-listings-and-ads-category', + 'path' => '/google/settings/edit-phone-number', + 'id' => 'google-settings-phone-number', + ] + ); + wc_admin_register_page( + [ + 'title' => __( 'Settings', 'google-listings-and-ads' ), + 'parent' => 'google-listings-and-ads-category', + 'path' => '/google/settings/edit-store-address', + 'id' => 'google-settings-store-address', + ] + ); + wc_admin_register_page( + [ + 'title' => __( 'Settings', 'google-listings-and-ads' ), + 'parent' => 'google-listings-and-ads-category', + 'path' => '/google/settings/reconnect-accounts', + 'id' => 'google-settings-reconnect-accounts', + ] + ); } ); } From 5590309563555930602f2bd92b8a25b977f3271e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Tue, 5 Oct 2021 18:43:57 +0200 Subject: [PATCH 2/2] Make settings subpages highlight Settings menu item, in the new WC Navigation. Addresses https://github.com/woocommerce/google-listings-and-ads/pull/1031#issuecomment-931503228. --- src/Menu/Settings.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Menu/Settings.php b/src/Menu/Settings.php index 72d47e0973..a725a09b40 100644 --- a/src/Menu/Settings.php +++ b/src/Menu/Settings.php @@ -29,6 +29,8 @@ function() { 'nav_args' => [ 'order' => 40, 'parent' => 'google-listings-and-ads-category', + // Highlight this menu item for other subpages. + 'matchExpression' => '/google/settings(/[^/]+)?', ], ] );