From e7e6687c3d7068193b4cac88f29897bf0caf8b9f Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Wed, 11 Sep 2024 12:48:04 +0530 Subject: [PATCH 01/14] First pass at appselectcontrol changes --- js/src/components/app-select-control/index.js | 21 ++++++++++++++++--- .../connect-ads/ads-account-select-control.js | 2 +- .../merchant-center-select-control/index.js | 1 + 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/js/src/components/app-select-control/index.js b/js/src/components/app-select-control/index.js index a0233d4905..1d2f0d567a 100644 --- a/js/src/components/app-select-control/index.js +++ b/js/src/components/app-select-control/index.js @@ -16,14 +16,29 @@ import './index.scss'; * it will be added to the container div's `className`, * so that you can further control its style. * - * @param {*} props + * @param {*} props The component props. + * @param {boolean} [props.selectSingleValue=false] Whether the select should show only one value. */ const AppSelectControl = ( props ) => { - const { className, ...rest } = props; + const { className, options, selectSingleValue, ...rest } = props; + const showSingleValue = selectSingleValue && options?.length === 1; + + const selectProps = showSingleValue ? + { + suffix: " ", + tabIndex: "-1", + style: { + pointerEvents: 'none', + }, + readOnly: true, + options, + ...rest, + } + : { options, ...rest }; return (
- +
); }; diff --git a/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js b/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js index e889074ef1..fb1bf3349e 100644 --- a/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js +++ b/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js @@ -27,7 +27,7 @@ const AdsAccountSelectControl = ( props ) => { } ) ), ]; - return ; + return ; }; export default AdsAccountSelectControl; diff --git a/js/src/components/merchant-center-select-control/index.js b/js/src/components/merchant-center-select-control/index.js index 3f5348a511..d00a1de12d 100644 --- a/js/src/components/merchant-center-select-control/index.js +++ b/js/src/components/merchant-center-select-control/index.js @@ -48,6 +48,7 @@ const MerchantCenterSelectControl = ( { return ( Date: Thu, 12 Sep 2024 10:14:12 +0530 Subject: [PATCH 02/14] Fix linter warnings --- js/src/components/app-select-control/index.js | 22 +++++++++---------- .../connect-ads/ads-account-select-control.js | 8 ++++++- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/js/src/components/app-select-control/index.js b/js/src/components/app-select-control/index.js index 1d2f0d567a..d2586ba9dc 100644 --- a/js/src/components/app-select-control/index.js +++ b/js/src/components/app-select-control/index.js @@ -23,17 +23,17 @@ const AppSelectControl = ( props ) => { const { className, options, selectSingleValue, ...rest } = props; const showSingleValue = selectSingleValue && options?.length === 1; - const selectProps = showSingleValue ? - { - suffix: " ", - tabIndex: "-1", - style: { - pointerEvents: 'none', - }, - readOnly: true, - options, - ...rest, - } + const selectProps = showSingleValue + ? { + suffix: ' ', + tabIndex: '-1', + style: { + pointerEvents: 'none', + }, + readOnly: true, + options, + ...rest, + } : { options, ...rest }; return ( diff --git a/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js b/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js index fb1bf3349e..4a6e961b80 100644 --- a/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js +++ b/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js @@ -27,7 +27,13 @@ const AdsAccountSelectControl = ( props ) => { } ) ), ]; - return ; + return ( + + ); }; export default AdsAccountSelectControl; From 5ab1b61bf2245467134441a7484039bc28ed7048 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 17 Sep 2024 20:00:58 +0400 Subject: [PATCH 03/14] Move AdsAccountSelectControl --- .../ads-account-select-control/index.js | 34 ++++++++++++++++ js/src/components/app-select-control/index.js | 33 +++++++++------- .../components/app-select-control/index.scss | 4 ++ .../connect-ads/ads-account-select-control.js | 39 ------------------- .../connect-ads/index.js | 2 +- 5 files changed, 58 insertions(+), 54 deletions(-) create mode 100644 js/src/components/ads-account-select-control/index.js delete mode 100644 js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js diff --git a/js/src/components/ads-account-select-control/index.js b/js/src/components/ads-account-select-control/index.js new file mode 100644 index 0000000000..d997b001a8 --- /dev/null +++ b/js/src/components/ads-account-select-control/index.js @@ -0,0 +1,34 @@ +/** + * External dependencies + */ +import { __, sprintf } from '@wordpress/i18n'; + +/** + * Internal dependencies + */ +import AppSelectControl from '.~/components/app-select-control'; +import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts'; + +const AdsAccountSelectControl = ( props ) => { + const { existingAccounts: accounts = [] } = useExistingGoogleAdsAccounts(); + + const options = accounts.map( ( acc ) => ( { + value: acc.id, + label: sprintf( + // translators: 1: account name, 2: account ID. + __( '%1$s (%2$s)', 'google-listings-and-ads' ), + acc.name, + acc.id + ), + } ) ); + + return ( + + ); +}; + +export default AdsAccountSelectControl; diff --git a/js/src/components/app-select-control/index.js b/js/src/components/app-select-control/index.js index d2586ba9dc..634894750c 100644 --- a/js/src/components/app-select-control/index.js +++ b/js/src/components/app-select-control/index.js @@ -20,24 +20,29 @@ import './index.scss'; * @param {boolean} [props.selectSingleValue=false] Whether the select should show only one value. */ const AppSelectControl = ( props ) => { - const { className, options, selectSingleValue, ...rest } = props; + const { className, options, selectSingleValue = false, ...rest } = props; const showSingleValue = selectSingleValue && options?.length === 1; - const selectProps = showSingleValue - ? { - suffix: ' ', - tabIndex: '-1', - style: { - pointerEvents: 'none', - }, - readOnly: true, - options, - ...rest, - } - : { options, ...rest }; + let selectProps = { + options, + ...rest, + }; + + if ( showSingleValue ) { + selectProps = { + ...selectProps, + suffix: ' ', + tabIndex: '-1', + readOnly: true, + }; + } return ( -
+
); diff --git a/js/src/components/app-select-control/index.scss b/js/src/components/app-select-control/index.scss index fdd98cc338..c50f101414 100644 --- a/js/src/components/app-select-control/index.scss +++ b/js/src/components/app-select-control/index.scss @@ -3,3 +3,7 @@ margin-bottom: 0; } } + +.app-select-control--has-single-value { + pointer-events: none; +} diff --git a/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js b/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js deleted file mode 100644 index 4a6e961b80..0000000000 --- a/js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js +++ /dev/null @@ -1,39 +0,0 @@ -/** - * External dependencies - */ -import { __, sprintf } from '@wordpress/i18n'; - -/** - * Internal dependencies - */ -import AppSelectControl from '.~/components/app-select-control'; - -const AdsAccountSelectControl = ( props ) => { - const { accounts, ...rest } = props; - - const options = [ - { - value: '', - label: __( 'Select one', 'google-listings-and-ads' ), - }, - ...accounts.map( ( acc ) => ( { - value: acc.id, - label: sprintf( - // translators: 1: account name, 2: account ID. - __( '%1$s (%2$s)', 'google-listings-and-ads' ), - acc.name, - acc.id - ), - } ) ), - ]; - - return ( - - ); -}; - -export default AdsAccountSelectControl; diff --git a/js/src/components/google-ads-account-card/connect-ads/index.js b/js/src/components/google-ads-account-card/connect-ads/index.js index 2ee5ff5771..4e7ff3bea0 100644 --- a/js/src/components/google-ads-account-card/connect-ads/index.js +++ b/js/src/components/google-ads-account-card/connect-ads/index.js @@ -19,7 +19,7 @@ import useApiFetchCallback from '.~/hooks/useApiFetchCallback'; import useDispatchCoreNotices from '.~/hooks/useDispatchCoreNotices'; import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount'; import useEventPropertiesFilter from '.~/hooks/useEventPropertiesFilter'; -import AdsAccountSelectControl from './ads-account-select-control'; +import AdsAccountSelectControl from '.~/components/ads-account-select-control'; import { useAppDispatch } from '.~/data'; import { FILTER_ONBOARDING } from '.~/utils/tracks'; import './index.scss'; From 2fb0353ee533f2af3197d2fb7722fae88c4f44e6 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 17 Sep 2024 23:59:26 +0400 Subject: [PATCH 04/14] Update Jest tests. --- .../ads-account-select-control/index.js | 23 ++++++++++++-- .../connect-ads/index.js | 1 - .../connect-ads/index.test.js | 30 +++++++++---------- .../merchant-center-select-control/index.js | 2 +- 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/js/src/components/ads-account-select-control/index.js b/js/src/components/ads-account-select-control/index.js index d997b001a8..27482d7f0b 100644 --- a/js/src/components/ads-account-select-control/index.js +++ b/js/src/components/ads-account-select-control/index.js @@ -2,6 +2,7 @@ * External dependencies */ import { __, sprintf } from '@wordpress/i18n'; +import { useEffect } from '@wordpress/element'; /** * Internal dependencies @@ -9,7 +10,17 @@ import { __, sprintf } from '@wordpress/i18n'; import AppSelectControl from '.~/components/app-select-control'; import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts'; -const AdsAccountSelectControl = ( props ) => { +/** + * @param {Object} props The component props + * @param {string} [props.value] The selected value. IF no value is defined, then the first option is selected and onChange function is triggered. + * @param {Function} [props.onChange] Callback when the select value changes. + * @return {JSX.Element} An enhanced AppSelectControl component. + */ +const AdsAccountSelectControl = ( { + value, + onChange = () => {}, + ...props +} ) => { const { existingAccounts: accounts = [] } = useExistingGoogleAdsAccounts(); const options = accounts.map( ( acc ) => ( { @@ -22,10 +33,18 @@ const AdsAccountSelectControl = ( props ) => { ), } ) ); + useEffect( () => { + // Triggers the onChange event in order to pre-select the initial value + if ( value === undefined ) { + onChange( options[ 0 ]?.value ); + } + }, [ options, onChange, value ] ); + return ( ); diff --git a/js/src/components/google-ads-account-card/connect-ads/index.js b/js/src/components/google-ads-account-card/connect-ads/index.js index 4e7ff3bea0..89c5dad133 100644 --- a/js/src/components/google-ads-account-card/connect-ads/index.js +++ b/js/src/components/google-ads-account-card/connect-ads/index.js @@ -120,7 +120,6 @@ const ConnectAds = ( props ) => { ) } diff --git a/js/src/components/google-ads-account-card/connect-ads/index.test.js b/js/src/components/google-ads-account-card/connect-ads/index.test.js index 5bcb0d32df..d685d3a853 100644 --- a/js/src/components/google-ads-account-card/connect-ads/index.test.js +++ b/js/src/components/google-ads-account-card/connect-ads/index.test.js @@ -15,6 +15,7 @@ import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount'; import { useAppDispatch } from '.~/data'; import { FILTER_ONBOARDING } from '.~/utils/tracks'; import expectComponentToRecordEventWithFilteredProperties from '.~/tests/expectComponentToRecordEventWithFilteredProperties'; +import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts'; jest.mock( '.~/hooks/useApiFetchCallback', () => jest.fn().mockName( 'useApiFetchCallback' ) @@ -24,6 +25,10 @@ jest.mock( '.~/hooks/useGoogleAdsAccount', () => jest.fn().mockName( 'useGoogleAdsAccount' ) ); +jest.mock( '.~/hooks/useExistingGoogleAdsAccounts', () => + jest.fn().mockName( 'useExistingGoogleAdsAccounts' ) +); + jest.mock( '.~/data', () => ( { ...jest.requireActual( '.~/data' ), useAppDispatch: jest.fn(), @@ -64,6 +69,10 @@ describe( 'ConnectAds', () => { .mockName( 'refetchGoogleAdsAccount' ), } ); + useExistingGoogleAdsAccounts.mockReturnValue( { + existingAccounts: accounts, + } ); + fetchGoogleAdsAccountStatus = jest .fn() .mockName( 'fetchGoogleAdsAccountStatus' ); @@ -76,11 +85,7 @@ describe( 'ConnectAds', () => { it( 'should render the given accounts in a selection', () => { render( ); - expect( screen.getByRole( 'combobox' ) ).toBeInTheDocument(); - expect( - screen.getByRole( 'option', { name: 'Select one' } ) - ).toBeInTheDocument(); expect( screen.getByRole( 'option', { name: 'Account A (1)' } ) ).toBeInTheDocument(); @@ -115,22 +120,15 @@ describe( 'ConnectAds', () => { expect( onCreateNew ).toHaveBeenCalledTimes( 1 ); } ); - it( 'should disable the "Connect" button when no account is selected', async () => { - const user = userEvent.setup(); + it( 'should disable the "Connect" button when there are no accounts', async () => { + useExistingGoogleAdsAccounts.mockReturnValue( { + existingAccounts: [], + } ); - render( ); - const combobox = screen.getByRole( 'combobox' ); + render( ); const button = getConnectButton(); expect( button ).toBeDisabled(); - - await user.selectOptions( combobox, '1' ); - - expect( button ).toBeEnabled(); - - await user.selectOptions( combobox, '' ); - - expect( button ).toBeDisabled(); } ); it( 'should render a connecting state and disable the button to switch to account creation after clicking the "Connect" button', async () => { diff --git a/js/src/components/merchant-center-select-control/index.js b/js/src/components/merchant-center-select-control/index.js index d00a1de12d..5933296742 100644 --- a/js/src/components/merchant-center-select-control/index.js +++ b/js/src/components/merchant-center-select-control/index.js @@ -48,9 +48,9 @@ const MerchantCenterSelectControl = ( { return ( ); From fd9f50a0e25808c9dd588dc8fa9ccaf960e65613 Mon Sep 17 00:00:00 2001 From: asvinb Date: Wed, 18 Sep 2024 00:01:10 +0400 Subject: [PATCH 05/14] Update label. --- .../components/google-ads-account-card/connect-ads/index.js | 2 +- js/src/components/google-mc-account-card/connect-mc/index.js | 2 +- tests/e2e/specs/setup-mc/step-1-accounts.test.js | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/js/src/components/google-ads-account-card/connect-ads/index.js b/js/src/components/google-ads-account-card/connect-ads/index.js index 89c5dad133..e6d407c51d 100644 --- a/js/src/components/google-ads-account-card/connect-ads/index.js +++ b/js/src/components/google-ads-account-card/connect-ads/index.js @@ -95,7 +95,7 @@ const ConnectAds = ( props ) => { { __( - 'Select an existing account', + 'Connect to an existing account', 'google-listings-and-ads' ) } diff --git a/js/src/components/google-mc-account-card/connect-mc/index.js b/js/src/components/google-mc-account-card/connect-mc/index.js index 6215fa9640..a47ccfa5fc 100644 --- a/js/src/components/google-mc-account-card/connect-mc/index.js +++ b/js/src/components/google-mc-account-card/connect-mc/index.js @@ -98,7 +98,7 @@ const ConnectMC = () => { { __( - 'Select an existing account', + 'Connect to an existing account', 'google-listings-and-ads' ) } diff --git a/tests/e2e/specs/setup-mc/step-1-accounts.test.js b/tests/e2e/specs/setup-mc/step-1-accounts.test.js index 6e84aa2ab1..f56c711a97 100644 --- a/tests/e2e/specs/setup-mc/step-1-accounts.test.js +++ b/tests/e2e/specs/setup-mc/step-1-accounts.test.js @@ -612,11 +612,11 @@ test.describe( 'Set up accounts', () => { } ); test.describe( 'connect to an existing account', () => { - test( 'should see "Select an existing account" title', async () => { + test( 'should see "Connect to an existing account" title', async () => { const selectAccountTitle = setUpAccountsPage.getSelectExistingMCAccountTitle(); await expect( selectAccountTitle ).toContainText( - 'Select an existing account' + 'Connect to an existing account' ); } ); From a17ce782e27b61c3899d617b8ff897df2a6e291c Mon Sep 17 00:00:00 2001 From: asvinb Date: Wed, 18 Sep 2024 00:42:02 +0400 Subject: [PATCH 06/14] Fix e2e tests. --- .../specs/add-paid-campaigns/add-paid-campaigns.test.js | 8 -------- tests/e2e/specs/setup-mc/step-1-accounts.test.js | 6 ------ 2 files changed, 14 deletions(-) diff --git a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js index 805f77e798..404237869d 100644 --- a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js +++ b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js @@ -246,18 +246,10 @@ test.describe( 'Set up Ads account', () => { test( 'Select one existing account', async () => { const adsAccountSelected = `${ ADS_ACCOUNTS[ 1 ].id }`; - await expect( - setupAdsAccounts.getConnectAdsButton() - ).toBeDisabled(); - await setupAdsAccounts.selectAnExistingAdsAccount( adsAccountSelected ); - await expect( - setupAdsAccounts.getConnectAdsButton() - ).toBeEnabled(); - //Intercept Ads connection request const connectAdsAccountRequest = setupAdsAccounts.registerConnectAdsAccountRequests( diff --git a/tests/e2e/specs/setup-mc/step-1-accounts.test.js b/tests/e2e/specs/setup-mc/step-1-accounts.test.js index f56c711a97..73399702c0 100644 --- a/tests/e2e/specs/setup-mc/step-1-accounts.test.js +++ b/tests/e2e/specs/setup-mc/step-1-accounts.test.js @@ -340,12 +340,6 @@ test.describe( 'Set up accounts', () => { await expect( select ).toBeVisible(); } ); - test( 'should see connect button is disabled when no ads account is selected', async () => { - const button = setupAdsAccountPage.getConnectAdsButton(); - await expect( button ).toBeVisible(); - await expect( button ).toBeDisabled(); - } ); - test( 'should see connect button is enabled when an ads account is selected', async () => { await setupAdsAccountPage.selectAnExistingAdsAccount( '23456' From 6342061bffc02a375f8a6fc35489ada481fbb9b0 Mon Sep 17 00:00:00 2001 From: asvinb Date: Wed, 18 Sep 2024 21:06:57 +0400 Subject: [PATCH 07/14] Do not use translation. --- js/src/components/ads-account-select-control/index.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/js/src/components/ads-account-select-control/index.js b/js/src/components/ads-account-select-control/index.js index 27482d7f0b..4497cfcf98 100644 --- a/js/src/components/ads-account-select-control/index.js +++ b/js/src/components/ads-account-select-control/index.js @@ -1,7 +1,6 @@ /** * External dependencies */ -import { __, sprintf } from '@wordpress/i18n'; import { useEffect } from '@wordpress/element'; /** @@ -25,12 +24,7 @@ const AdsAccountSelectControl = ( { const options = accounts.map( ( acc ) => ( { value: acc.id, - label: sprintf( - // translators: 1: account name, 2: account ID. - __( '%1$s (%2$s)', 'google-listings-and-ads' ), - acc.name, - acc.id - ), + label: `${ acc.name } (${ acc.id })`, } ) ); useEffect( () => { From d575ba68b8e8dd8f755a07a21659bfff1b285bfe Mon Sep 17 00:00:00 2001 From: asvinb Date: Mon, 30 Sep 2024 19:02:34 +0400 Subject: [PATCH 08/14] Move auto select logic to AppSelectControl. --- .../ads-account-select-control/index.js | 15 ++------- js/src/components/app-select-control/index.js | 31 ++++++++++++++++--- .../merchant-center-select-control/index.js | 10 +----- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/js/src/components/ads-account-select-control/index.js b/js/src/components/ads-account-select-control/index.js index 4497cfcf98..9b78dcaf93 100644 --- a/js/src/components/ads-account-select-control/index.js +++ b/js/src/components/ads-account-select-control/index.js @@ -1,8 +1,3 @@ -/** - * External dependencies - */ -import { useEffect } from '@wordpress/element'; - /** * Internal dependencies */ @@ -27,18 +22,12 @@ const AdsAccountSelectControl = ( { label: `${ acc.name } (${ acc.id })`, } ) ); - useEffect( () => { - // Triggers the onChange event in order to pre-select the initial value - if ( value === undefined ) { - onChange( options[ 0 ]?.value ); - } - }, [ options, onChange, value ] ); - return ( ); diff --git a/js/src/components/app-select-control/index.js b/js/src/components/app-select-control/index.js index 634894750c..31562dec4c 100644 --- a/js/src/components/app-select-control/index.js +++ b/js/src/components/app-select-control/index.js @@ -2,6 +2,7 @@ * External dependencies */ import { SelectControl } from '@wordpress/components'; +import { useEffect } from '@wordpress/element'; import classNames from 'classnames'; /** @@ -17,18 +18,38 @@ import './index.scss'; * so that you can further control its style. * * @param {*} props The component props. - * @param {boolean} [props.selectSingleValue=false] Whether the select should show only one value. + * @param {boolean} [props.autoSelectFirstOption=false] Whether the automatically select the first option. */ const AppSelectControl = ( props ) => { - const { className, options, selectSingleValue = false, ...rest } = props; - const showSingleValue = selectSingleValue && options?.length === 1; + const { + className, + options, + onChange, + value, + autoSelectFirstOption = false, + ...rest + } = props; + const hasSingleValueStyle = autoSelectFirstOption && options?.length === 1; + + useEffect( () => { + // Triggers the onChange event to set the initial value for the select + if ( + autoSelectFirstOption && + options?.length > 0 && + value === undefined + ) { + onChange( options[ 0 ].value ); + } + }, [ autoSelectFirstOption, onChange, options, value ] ); let selectProps = { options, + value, + onChange, ...rest, }; - if ( showSingleValue ) { + if ( hasSingleValueStyle ) { selectProps = { ...selectProps, suffix: ' ', @@ -40,7 +61,7 @@ const AppSelectControl = ( props ) => { return (
diff --git a/js/src/components/merchant-center-select-control/index.js b/js/src/components/merchant-center-select-control/index.js index 5933296742..addfdbc69c 100644 --- a/js/src/components/merchant-center-select-control/index.js +++ b/js/src/components/merchant-center-select-control/index.js @@ -2,7 +2,6 @@ * External dependencies */ import { __, sprintf } from '@wordpress/i18n'; -import { useEffect } from '@wordpress/element'; /** * Internal dependencies @@ -38,19 +37,12 @@ const MerchantCenterSelectControl = ( { return a.label.localeCompare( b.label ); } ); - useEffect( () => { - // Triggers the onChange event in order to pre-select the initial value - if ( value === undefined ) { - onChange( options[ 0 ]?.value ); - } - }, [ options, onChange, value ] ); - return ( ); From 0eafa455b538d68fef7f9446569994aac4476a25 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 1 Oct 2024 15:37:07 +0400 Subject: [PATCH 09/14] Address CR feedback. --- .../ads-account-select-control/index.js | 10 +----- js/src/components/app-select-control/index.js | 34 +++++++++++++------ .../merchant-center-select-control/index.js | 10 +----- 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/js/src/components/ads-account-select-control/index.js b/js/src/components/ads-account-select-control/index.js index 9b78dcaf93..af0e45e0b0 100644 --- a/js/src/components/ads-account-select-control/index.js +++ b/js/src/components/ads-account-select-control/index.js @@ -6,15 +6,9 @@ import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts' /** * @param {Object} props The component props - * @param {string} [props.value] The selected value. IF no value is defined, then the first option is selected and onChange function is triggered. - * @param {Function} [props.onChange] Callback when the select value changes. * @return {JSX.Element} An enhanced AppSelectControl component. */ -const AdsAccountSelectControl = ( { - value, - onChange = () => {}, - ...props -} ) => { +const AdsAccountSelectControl = ( props ) => { const { existingAccounts: accounts = [] } = useExistingGoogleAdsAccounts(); const options = accounts.map( ( acc ) => ( { @@ -25,8 +19,6 @@ const AdsAccountSelectControl = ( { return ( diff --git a/js/src/components/app-select-control/index.js b/js/src/components/app-select-control/index.js index 31562dec4c..bbe4ec1609 100644 --- a/js/src/components/app-select-control/index.js +++ b/js/src/components/app-select-control/index.js @@ -2,8 +2,9 @@ * External dependencies */ import { SelectControl } from '@wordpress/components'; -import { useEffect } from '@wordpress/element'; +import { useEffect, useState } from '@wordpress/element'; import classNames from 'classnames'; +import { noop } from 'lodash'; /** * Internal dependencies @@ -13,34 +14,47 @@ import './index.scss'; /** * Renders a `@wordpress/components`'s `SelectControl` with margin-bottom removed. * - * If you provide `className` via props, - * it will be added to the container div's `className`, - * so that you can further control its style. - * * @param {*} props The component props. - * @param {boolean} [props.autoSelectFirstOption=false] Whether the automatically select the first option. + * @param {Array} props.options Array of options for the select dropdown. Each option should be an object containing `label` and `value` properties. + * @param {string} [props.className] Additional classname to further control the style of the component. + * @param {Function} [props.onChange=noop] Callback function triggered when the selected value changes. Receives the new value as an argument. + * @param {string} [props.value] The selected value. If no value is defined, the first option is selected and `onChange` is triggered when `autoSelectFirstOption` is true. + * @param {boolean} [props.autoSelectFirstOption=false] If true, automatically triggers the onChange callback with the first option as value when no value is provided. If only one option is available, the select is also disabled to prevent user interaction. + * @param {*} [props.rest] Additional props passed to the `SelectControl` component. */ const AppSelectControl = ( props ) => { const { - className, options, - onChange, + className, + onChange = noop, value, autoSelectFirstOption = false, ...rest } = props; + const [ hasTriggerredOnChangeOnMount, setHasTriggerredOnChangeOnMount ] = + useState( false ); const hasSingleValueStyle = autoSelectFirstOption && options?.length === 1; useEffect( () => { - // Triggers the onChange event to set the initial value for the select + if ( hasTriggerredOnChangeOnMount ) { + return; + } + if ( autoSelectFirstOption && options?.length > 0 && value === undefined ) { onChange( options[ 0 ].value ); + setHasTriggerredOnChangeOnMount( true ); } - }, [ autoSelectFirstOption, onChange, options, value ] ); + }, [ + autoSelectFirstOption, + onChange, + options, + value, + hasTriggerredOnChangeOnMount, + ] ); let selectProps = { options, diff --git a/js/src/components/merchant-center-select-control/index.js b/js/src/components/merchant-center-select-control/index.js index addfdbc69c..500fe68834 100644 --- a/js/src/components/merchant-center-select-control/index.js +++ b/js/src/components/merchant-center-select-control/index.js @@ -11,15 +11,9 @@ import AppSelectControl from '.~/components/app-select-control'; /** * @param {Object} props The component props - * @param {string} [props.value] The selected value. IF no value is defined, then the first option is selected and onChange function is triggered. - * @param {Function} [props.onChange] Callback when the select value changes. * @return {JSX.Element} An enhanced AppSelectControl component. */ -const MerchantCenterSelectControl = ( { - value, - onChange = () => {}, - ...props -} ) => { +const MerchantCenterSelectControl = ( props ) => { const { data: existingAccounts = [] } = useExistingGoogleMCAccounts(); const options = existingAccounts.map( ( acc ) => { return { @@ -40,8 +34,6 @@ const MerchantCenterSelectControl = ( { return ( From 8c43ac67f7bf2047098eb0de5d3433bf2d5547cb Mon Sep 17 00:00:00 2001 From: asvinb Date: Wed, 2 Oct 2024 12:53:22 +0400 Subject: [PATCH 10/14] Use useRef to prevent re renders. --- js/src/components/app-select-control/index.js | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/js/src/components/app-select-control/index.js b/js/src/components/app-select-control/index.js index bbe4ec1609..dca828bf41 100644 --- a/js/src/components/app-select-control/index.js +++ b/js/src/components/app-select-control/index.js @@ -2,7 +2,7 @@ * External dependencies */ import { SelectControl } from '@wordpress/components'; -import { useEffect, useState } from '@wordpress/element'; +import { useEffect, useRef } from '@wordpress/element'; import classNames from 'classnames'; import { noop } from 'lodash'; @@ -31,30 +31,30 @@ const AppSelectControl = ( props ) => { autoSelectFirstOption = false, ...rest } = props; - const [ hasTriggerredOnChangeOnMount, setHasTriggerredOnChangeOnMount ] = - useState( false ); - const hasSingleValueStyle = autoSelectFirstOption && options?.length === 1; + // Maintain refs to prevent rerender + const onChangeRef = useRef(); + const valueRef = useRef(); + const autoSelectFirstOptionRef = useRef(); + const optionsRef = useRef(); + + // Update refs if any of the dependencies change useEffect( () => { - if ( hasTriggerredOnChangeOnMount ) { - return; - } + onChangeRef.current = onChange; + valueRef.current = value; + autoSelectFirstOptionRef.current = autoSelectFirstOption; + optionsRef.current = options; + }, [ onChange, value, autoSelectFirstOption, options ] ); + useEffect( () => { if ( - autoSelectFirstOption && - options?.length > 0 && - value === undefined + autoSelectFirstOptionRef.current && + optionsRef.current?.length > 0 && + valueRef.current === undefined ) { - onChange( options[ 0 ].value ); - setHasTriggerredOnChangeOnMount( true ); + onChangeRef.current( optionsRef.current[ 0 ].value ); } - }, [ - autoSelectFirstOption, - onChange, - options, - value, - hasTriggerredOnChangeOnMount, - ] ); + }, [] ); let selectProps = { options, @@ -63,6 +63,7 @@ const AppSelectControl = ( props ) => { ...rest, }; + const hasSingleValueStyle = autoSelectFirstOption && options?.length === 1; if ( hasSingleValueStyle ) { selectProps = { ...selectProps, From 004c9f650cb88930479b6b7e0f295e53edaee03c Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 4 Oct 2024 15:42:21 +0400 Subject: [PATCH 11/14] Check for undefined values. --- js/src/components/ads-account-select-control/index.js | 2 +- js/src/components/merchant-center-select-control/index.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/js/src/components/ads-account-select-control/index.js b/js/src/components/ads-account-select-control/index.js index af0e45e0b0..fc82646301 100644 --- a/js/src/components/ads-account-select-control/index.js +++ b/js/src/components/ads-account-select-control/index.js @@ -11,7 +11,7 @@ import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts' const AdsAccountSelectControl = ( props ) => { const { existingAccounts: accounts = [] } = useExistingGoogleAdsAccounts(); - const options = accounts.map( ( acc ) => ( { + const options = accounts?.map( ( acc ) => ( { value: acc.id, label: `${ acc.name } (${ acc.id })`, } ) ); diff --git a/js/src/components/merchant-center-select-control/index.js b/js/src/components/merchant-center-select-control/index.js index 500fe68834..9a6c22175a 100644 --- a/js/src/components/merchant-center-select-control/index.js +++ b/js/src/components/merchant-center-select-control/index.js @@ -15,7 +15,7 @@ import AppSelectControl from '.~/components/app-select-control'; */ const MerchantCenterSelectControl = ( props ) => { const { data: existingAccounts = [] } = useExistingGoogleMCAccounts(); - const options = existingAccounts.map( ( acc ) => { + const options = existingAccounts?.map( ( acc ) => { return { value: acc.id, label: sprintf( From 08b16e067128362d3053e50fb1e8c7c7ebf9f204 Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 4 Oct 2024 16:35:00 +0400 Subject: [PATCH 12/14] Make better use of useRef. --- js/src/components/app-select-control/index.js | 30 ++++++------------- .../merchant-center-select-control/index.js | 2 +- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/js/src/components/app-select-control/index.js b/js/src/components/app-select-control/index.js index dca828bf41..191018f3cc 100644 --- a/js/src/components/app-select-control/index.js +++ b/js/src/components/app-select-control/index.js @@ -24,37 +24,25 @@ import './index.scss'; */ const AppSelectControl = ( props ) => { const { - options, + options = [], className, onChange = noop, value, autoSelectFirstOption = false, ...rest } = props; + const shouldAutoSelectOnceRef = useRef( autoSelectFirstOption === true ); - // Maintain refs to prevent rerender - const onChangeRef = useRef(); - const valueRef = useRef(); - const autoSelectFirstOptionRef = useRef(); - const optionsRef = useRef(); - - // Update refs if any of the dependencies change useEffect( () => { - onChangeRef.current = onChange; - valueRef.current = value; - autoSelectFirstOptionRef.current = autoSelectFirstOption; - optionsRef.current = options; - }, [ onChange, value, autoSelectFirstOption, options ] ); + if ( ! shouldAutoSelectOnceRef.current ) { + return; + } - useEffect( () => { - if ( - autoSelectFirstOptionRef.current && - optionsRef.current?.length > 0 && - valueRef.current === undefined - ) { - onChangeRef.current( optionsRef.current[ 0 ].value ); + if ( value === undefined && options.length ) { + shouldAutoSelectOnceRef.current = false; + onChange( options[ 0 ].value ); } - }, [] ); + }, [ value, options, onChange ] ); let selectProps = { options, diff --git a/js/src/components/merchant-center-select-control/index.js b/js/src/components/merchant-center-select-control/index.js index 9a6c22175a..a308d4544f 100644 --- a/js/src/components/merchant-center-select-control/index.js +++ b/js/src/components/merchant-center-select-control/index.js @@ -27,7 +27,7 @@ const MerchantCenterSelectControl = ( props ) => { ), }; } ); - options.sort( ( a, b ) => { + options?.sort( ( a, b ) => { return a.label.localeCompare( b.label ); } ); From f29035239778cb8abcea4324068c7810c82ea552 Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 4 Oct 2024 16:36:04 +0400 Subject: [PATCH 13/14] Updated JSDocs. --- js/src/components/app-select-control/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/js/src/components/app-select-control/index.js b/js/src/components/app-select-control/index.js index 191018f3cc..2d473529f7 100644 --- a/js/src/components/app-select-control/index.js +++ b/js/src/components/app-select-control/index.js @@ -18,8 +18,8 @@ import './index.scss'; * @param {Array} props.options Array of options for the select dropdown. Each option should be an object containing `label` and `value` properties. * @param {string} [props.className] Additional classname to further control the style of the component. * @param {Function} [props.onChange=noop] Callback function triggered when the selected value changes. Receives the new value as an argument. - * @param {string} [props.value] The selected value. If no value is defined, the first option is selected and `onChange` is triggered when `autoSelectFirstOption` is true. - * @param {boolean} [props.autoSelectFirstOption=false] If true, automatically triggers the onChange callback with the first option as value when no value is provided. If only one option is available, the select is also disabled to prevent user interaction. + * @param {string} [props.value] The currently selected value. This component should be used as a controlled component. A special case is that after mounting, when `autoSelectFirstOption` is true and `value` is undefined, it tries to call back `onChange` once to select the first option so that the `value` can be consistent with the `` element's own value. diff --git a/js/src/components/merchant-center-select-control/index.js b/js/src/components/merchant-center-select-control/index.js index a308d4544f..9e2b448f3a 100644 --- a/js/src/components/merchant-center-select-control/index.js +++ b/js/src/components/merchant-center-select-control/index.js @@ -14,7 +14,7 @@ import AppSelectControl from '.~/components/app-select-control'; * @return {JSX.Element} An enhanced AppSelectControl component. */ const MerchantCenterSelectControl = ( props ) => { - const { data: existingAccounts = [] } = useExistingGoogleMCAccounts(); + const { data: existingAccounts } = useExistingGoogleMCAccounts(); const options = existingAccounts?.map( ( acc ) => { return { value: acc.id,