Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2593: Update AppSelectControl to single select #2608

Merged
28 changes: 28 additions & 0 deletions js/src/components/ads-account-select-control/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Internal dependencies
*/
import AppSelectControl from '.~/components/app-select-control';
import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts';

/**
* @param {Object} props The component props
* @return {JSX.Element} An enhanced AppSelectControl component.
*/
const AdsAccountSelectControl = ( props ) => {
const { existingAccounts: accounts = [] } = useExistingGoogleAdsAccounts();
eason9487 marked this conversation as resolved.
Show resolved Hide resolved

const options = accounts.map( ( acc ) => ( {
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
value: acc.id,
label: `${ acc.name } (${ acc.id })`,
} ) );

return (
<AppSelectControl
options={ options }
autoSelectFirstOption
{ ...props }
/>
);
};

export default AdsAccountSelectControl;
72 changes: 64 additions & 8 deletions js/src/components/app-select-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
* External dependencies
*/
import { SelectControl } from '@wordpress/components';
import { useEffect, useRef } from '@wordpress/element';
import classNames from 'classnames';
import { noop } from 'lodash';

/**
* Internal dependencies
Expand All @@ -12,18 +14,72 @@
/**
* 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
* @param {*} props The component props.
* @param {Array} props.options Array of options for the select dropdown. Each option should be an object containing `label` and `value` properties.
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
* @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.
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
* @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.
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
* @param {*} [props.rest] Additional props passed to the `SelectControl` component.
*/
const AppSelectControl = ( props ) => {
const { className, ...rest } = props;
const {
options,
className,
onChange = noop,
value,
autoSelectFirstOption = false,
...rest
} = props;

// 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 ] );

useEffect( () => {
if (
autoSelectFirstOptionRef.current &&
optionsRef.current?.length > 0 &&
valueRef.current === undefined
) {
onChangeRef.current( optionsRef.current[ 0 ].value );
}
}, [] );
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
eason9487 marked this conversation as resolved.
Show resolved Hide resolved

let selectProps = {
options,
value,
onChange,
...rest,
};

const hasSingleValueStyle = autoSelectFirstOption && options?.length === 1;
if ( hasSingleValueStyle ) {
selectProps = {

Check warning on line 68 in js/src/components/app-select-control/index.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/app-select-control/index.js#L68

Added line #L68 was not covered by tests
...selectProps,
suffix: ' ',
tabIndex: '-1',
readOnly: true,
};
}

return (
<div className={ classNames( 'app-select-control', className ) }>
<SelectControl { ...rest } />
<div
className={ classNames( 'app-select-control', className, {
'app-select-control--has-single-value': hasSingleValueStyle,
} ) }
>
<SelectControl { ...selectProps } />
</div>
);
};
Expand Down
4 changes: 4 additions & 0 deletions js/src/components/app-select-control/index.scss
asvinb marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,7 @@
margin-bottom: 0;
}
}

.app-select-control--has-single-value {
pointer-events: none;
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -95,7 +95,7 @@ const ConnectAds = ( props ) => {
<Section.Card.Body>
<Subsection.Title>
{ __(
'Select an existing account',
'Connect to an existing account',
'google-listings-and-ads'
) }
</Subsection.Title>
Expand All @@ -120,7 +120,6 @@ const ConnectAds = ( props ) => {
) }
<ContentButtonLayout>
<AdsAccountSelectControl
accounts={ accounts }
value={ value }
onChange={ setValue }
/>
Expand Down
30 changes: 14 additions & 16 deletions js/src/components/google-ads-account-card/connect-ads/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' )
Expand All @@ -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(),
Expand Down Expand Up @@ -64,6 +69,10 @@ describe( 'ConnectAds', () => {
.mockName( 'refetchGoogleAdsAccount' ),
} );

useExistingGoogleAdsAccounts.mockReturnValue( {
existingAccounts: accounts,
} );

fetchGoogleAdsAccountStatus = jest
.fn()
.mockName( 'fetchGoogleAdsAccountStatus' );
Expand All @@ -76,11 +85,7 @@ describe( 'ConnectAds', () => {

it( 'should render the given accounts in a selection', () => {
render( <ConnectAds accounts={ accounts } /> );

expect( screen.getByRole( 'combobox' ) ).toBeInTheDocument();
expect(
screen.getByRole( 'option', { name: 'Select one' } )
).toBeInTheDocument();
expect(
screen.getByRole( 'option', { name: 'Account A (1)' } )
).toBeInTheDocument();
Expand Down Expand Up @@ -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( <ConnectAds accounts={ accounts } /> );
const combobox = screen.getByRole( 'combobox' );
render( <ConnectAds accounts={ [] } /> );
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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ const ConnectMC = () => {
<Section.Card.Body>
<Subsection.Title>
{ __(
'Select an existing account',
'Connect to an existing account',
'google-listings-and-ads'
) }
</Subsection.Title>
Expand Down
19 changes: 2 additions & 17 deletions js/src/components/merchant-center-select-control/index.js
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
* External dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { useEffect } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -12,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();
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
const options = existingAccounts.map( ( acc ) => {
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
return {
Expand All @@ -38,18 +31,10 @@ 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 (
<AppSelectControl
options={ options }
onChange={ onChange }
value={ value }
autoSelectFirstOption
{ ...props }
/>
);
Expand Down
8 changes: 0 additions & 8 deletions tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down