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

Implement PhoneNumberCard and ContactInformation components for contact info #917

Merged
merged 11 commits into from
Aug 4, 2021
Merged
19 changes: 15 additions & 4 deletions js/src/components/account-card/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import classnames from 'classnames';
import { Flex, FlexItem, FlexBlock } from '@wordpress/components';
import GridiconPhone from 'gridicons/dist/phone';

/**
* Internal dependencies
Expand All @@ -25,11 +27,12 @@ const googleLogoURL =
*/
export const APPEARANCE = {
GOOGLE: 'google',
PHONE: 'phone',
};

const appearanceDict = {
[ APPEARANCE.GOOGLE ]: {
logo: (
icon: (
<img
src={ googleLogoURL }
alt={ __( 'Google Logo', 'google-listings-and-ads' ) }
Expand All @@ -39,30 +42,38 @@ const appearanceDict = {
),
title: __( 'Google account', 'google-listings-and-ads' ),
},
[ APPEARANCE.PHONE ]: {
icon: <GridiconPhone size={ 32 } />,
title: __( 'Phone number', 'google-listings-and-ads' ),
},
};

/**
* Renders a Card component with account info and status.
*
* @param {Object} props React props.
* @param {string} [props.className] Additional CSS class name to be appended.
* @param {APPEARANCE} props.appearance Kind of account to indicate the card appearance.
* @param {JSX.Element} props.description Content below the card title.
* @param {boolean} [props.hideIcon=false] Whether hide the leading icon.
* @param {JSX.Element} [props.indicator] Indicator of actions or status on the right side of the card.
* @param {Array<JSX.Element>} [props.children] Children to be rendered if needs more content within the card.
*/
export default function AccountCard( {
className,
appearance,
description,
hideIcon = false,
indicator,
children,
} ) {
const { logo, title } = appearanceDict[ appearance ];
const { icon, title } = appearanceDict[ appearance ];

return (
<Section.Card className="gla-account-card">
<Section.Card className={ classnames( 'gla-account-card', className ) }>
<Section.Card.Body>
<Flex gap={ 4 }>
<FlexItem>{ logo }</FlexItem>
{ ! hideIcon && <FlexItem>{ icon }</FlexItem> }
<FlexBlock>
<Subsection.Title>{ title }</Subsection.Title>
<div>{ description }</div>
Expand Down
67 changes: 67 additions & 0 deletions js/src/components/contact-information/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* External dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import useGoogleMCPhoneNumber from '.~/hooks/useGoogleMCPhoneNumber';
import Section from '.~/wcdl/section';
import VerticalGapLayout from '.~/components/vertical-gap-layout';
import AppDocumentationLink from '.~/components/app-documentation-link';
import AppSpinner from '.~/components/app-spinner';
import PhoneNumberCard from './phone-number-card';

const description = __(
'Your contact information is required by Google for verification purposes. It will be shared with the Google Merchant Center and will not be displayed to customers.',
'google-listings-and-ads'
);

const mcTitle = __( 'Enter contact information', 'google-listings-and-ads' );
const settingsTitle = __( 'Contact information', 'google-listings-and-ads' );

export default function ContactInformation( { view, onPhoneNumberChange } ) {
const phone = useGoogleMCPhoneNumber();
const isSetupMC = view === 'setup-mc';

const initEditing = isSetupMC ? ! phone.data.isValid : true;
const title = isSetupMC ? mcTitle : settingsTitle;
const trackContext = isSetupMC
? 'setup-mc-contact-information'
: 'settings-contact-information';

return (
<Section
title={ title }
description={
<div>
<p>{ description }</p>
<p>
<AppDocumentationLink
context={ trackContext }
linkId="contact-information-read-more"
// TODO: [lite-contact-info] add link
href="https://example.com/"
>
{ __( 'Learn more', 'google-listings-and-ads' ) }
</AppDocumentationLink>
</p>
</div>
}
>
{ phone.loaded ? (
<VerticalGapLayout size="large">
<PhoneNumberCard
phoneNumber={ phone }
initEditing={ initEditing }
onPhoneNumberChange={ onPhoneNumberChange }
/>
<div>TODO: add store address card</div>
</VerticalGapLayout>
) : (
<AppSpinner />
) }
Comment on lines +53 to +64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of #917 (comment) I think this loading state handling is redundant, and delivers slightly worse UX.

We can remove the redundancy from ContactInformation and let the PhoneNumberCard handle loading state itself.

Suggested change
{ phone.loaded ? (
<VerticalGapLayout size="large">
<PhoneNumberCard
phoneNumber={ phone }
initEditing={ initEditing }
onPhoneNumberChange={ onPhoneNumberChange }
/>
<div>TODO: add store address card</div>
</VerticalGapLayout>
) : (
<AppSpinner />
) }
<VerticalGapLayout size="large">
<PhoneNumberCard
phoneNumber={ phone }
initEditing={ initEditing }
onPhoneNumberChange={ onPhoneNumberChange }
/>
<div>TODO: add store address card</div>
</VerticalGapLayout>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the change suggested above, we would have to change initEditing handling.
Personally, in ContactInformation I'd cater for isSetupMC only. Then let PhoneNumberCard reason about phone.data.isValid + some given property.
.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare this way to #917 (comment), it could only deliver the phone icon and "Phone number" title earlier about 0.5 ~ 1.5 seconds at the most. The rest elements are still needed to wait for data loaded. I'm not sure if that is worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say 0.5~1.5 s on a developer machine is a lot. Then imagine how long it could take on a low-end device with a slow network.

I'm not sure if that is worth it.

Personally, I see it the opposite, compare:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened another PR #934 to change the handling of loading states.

</Section>
);
}
143 changes: 143 additions & 0 deletions js/src/components/contact-information/phone-number-card.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
/**
* External dependencies
*/
import { getCountryCallingCode } from 'libphonenumber-js';
import { __ } from '@wordpress/i18n';
import { useState } from '@wordpress/element';
import { Flex, FlexItem, FlexBlock, CardDivider } from '@wordpress/components';
import { Spinner } from '@woocommerce/components';

/**
* Internal dependencies
*/
import useCountryCallingCodeOptions from '.~/hooks/useCountryCallingCodeOptions';
import Section from '.~/wcdl/section';
import SelectControl from '.~/wcdl/select-control';
import AppInputControl from '.~/components/app-input-control';
import AccountCard, { APPEARANCE } from '.~/components/account-card';
import AppButton from '.~/components/app-button';
import AppSpinner from '.~/components/app-spinner';
import './phone-number-card.scss';

const noop = () => {};

function PhoneNumberContent( {
initCountry,
initNationalNumber,
onPhoneNumberChange,
} ) {
const countryCallingCodeOptions = useCountryCallingCodeOptions();
const [ country, setCountry ] = useState( initCountry );
const [ number, setNumber ] = useState( initNationalNumber );

const handleChange = ( nextCountry, nextNumber ) => {
setCountry( nextCountry );
setNumber( nextNumber );

const countryCallingCode = nextCountry
? getCountryCallingCode( nextCountry )
: '';
onPhoneNumberChange( countryCallingCode, nextNumber, nextCountry );
};

const handleCountryChange = ( nextCountry ) =>
handleChange( nextCountry, number );

const handleNumberChange = ( nextNumber ) =>
handleChange( country, nextNumber );

return (
<Section.Card.Body>
<Flex gap={ 4 }>
<FlexItem>
<SelectControl
label={ __(
'Country code',
'google-listings-and-ads'
) }
isSearchable
excludeSelectedOptions={ false }
options={ countryCallingCodeOptions }
selected={ country }
onChange={ handleCountryChange }
/>
</FlexItem>
<FlexBlock>
<AppInputControl
label={ __(
'Phone number',
'google-listings-and-ads'
) }
value={ number }
onChange={ handleNumberChange }
/>
</FlexBlock>
</Flex>
</Section.Card.Body>
);
}

export default function PhoneNumberCard( {
phoneNumber,
isPreview,
initEditing,
onEditClick,
onPhoneNumberChange = noop,
} ) {
const [ isEditing, setEditing ] = useState( initEditing );
const { loaded, data } = phoneNumber;

const editButton = isEditing ? null : (
<AppButton
isSecondary
onClick={ () => {
if ( onEditClick ) {
onEditClick();
} else {
setEditing( true );
}
} }
>
{ __( 'Edit', 'google-listings-and-ads' ) }
</AppButton>
);

let description = null;
let phoneNumberContent = null;

if ( isEditing ) {
description = __(
'Please enter a phone number to be used for verification.',
'google-listings-and-ads'
);

if ( loaded ) {
phoneNumberContent = (
<>
<CardDivider />
<PhoneNumberContent
initCountry={ data.country }
initNationalNumber={ data.nationalNumber }
onPhoneNumberChange={ onPhoneNumberChange }
/>
</>
);
} else {
phoneNumberContent = <AppSpinner />;
}
} else {
description = loaded ? data.display : <Spinner />;
Comment on lines +114 to +129
Copy link
Member

@tomalec tomalec Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Having the preloading logic here and in https://github.com/woocommerce/google-listings-and-ads/pull/917/files#diff-51ff763be0b20871d193755311c5c437caafd6c4d46bb1413de599e1124a7714R53 looks like too much.
I think we can make containing element render this component. Then, this component could decide whether to render a scaffold with a tiny preloader in place of a phone number, or a fully prepared input control.

Also, in one case we use <Spinner> in the other <AppSpinner>. Is that on purpose?

Copy link
Member Author

@eason9487 eason9487 Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not quite sure of your meaning. Could you provide some code examples?

The logic is used for rendering different visual results.

Kapture 2021-08-03 at 11 36 33

The <Spinner> is shown in the first, and the <AppSpinner> is for the second one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of <Spinner> vs. <AppSpinner> now I got it, thanks. I agree we need both here. I got mislead by their names. I thought App- one is just profiled for our need, while they are in fact <InlineSpinenr> and <BlockSpinner>.

Copy link
Member

@tomalec tomalec Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💅 📜 For the block spinner <AppSpinner>, I'd adapt its position/margin/padding to make the card fixed height = do not jump once loaded.

Copy link
Member Author

@eason9487 eason9487 Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the loading UI in the later PR #921, and it shows a spinner with a card layout instead. The demo in another PR shows that change:

Kapture.2021-08-02.at.18.37.16.mp4

}

return (
<AccountCard
className="gla-phone-number-card"
appearance={ APPEARANCE.PHONE }
description={ description }
hideIcon={ isPreview }
indicator={ editButton }
>
{ phoneNumberContent }
</AccountCard>
);
}
42 changes: 42 additions & 0 deletions js/src/components/contact-information/phone-number-card.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
.gla-phone-number-card {
// Country code selector
.wcdl-select-control {
.wcdl-select-control__input {
margin-bottom: 0;
}

.woocommerce-select-control {
.components-base-control {
height: 36px;
border-color: $gray-600;
}

.woocommerce-select-control__control-input {
font-size: 13px;
color: $gray-900;
}

.woocommerce-select-control__listbox {
top: 37px;
}

.woocommerce-select-control__option {
min-height: 36px;
font-size: 13px;
}
}
}

// Phone number input
.components-input-control .components-input-control__container {
.components-input-control__input {
height: 36px;
font-size: 13px;
color: $gray-900;
}

.components-input-control__backdrop {
border-color: $gray-600;
}
}
}
4 changes: 4 additions & 0 deletions js/src/data/resolvers.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ export function* getExistingGoogleAdsAccounts() {
yield fetchExistingGoogleAdsAccounts();
}

export function* getGoogleMCPhoneNumber() {
// TODO: [lite-contact-info] integrate with API
}

export function* getCountries() {
yield fetchCountries();
}
Expand Down
9 changes: 9 additions & 0 deletions js/src/data/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ export const getExistingGoogleAdsAccounts = ( state ) => {
return state.mc.accounts.existing_ads;
};

const mockPhoneNumber = Math.random() > 0.5 ? '+12133734253' : '';
export const getGoogleMCPhoneNumber = () => {
// TODO: [lite-contact-info] integrate with API
return {
id: '123456789',
phone_number: mockPhoneNumber,
};
};

export const getCountries = ( state ) => {
return state.mc.countries;
};
Expand Down
33 changes: 33 additions & 0 deletions js/src/hooks/useCountryCallingCodeOptions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* External dependencies
*/
import { useMemo } from '@wordpress/element';
import { getCountries, getCountryCallingCode } from 'libphonenumber-js';

/**
* Internal dependencies
*/
import useCountryKeyNameMap from './useCountryKeyNameMap';

const toOption = ( country, countryCallingCode, countryName ) => ( {
key: country,
keywords: [ countryName, countryCallingCode, country ],
label: `${ countryName } (+${ countryCallingCode })`,
} );

export default function useCountryCallingCodeOptions() {
const countryNameDict = useCountryKeyNameMap();

return useMemo( () => {
return getCountries().reduce( ( acc, country ) => {
const countryName = countryNameDict[ country ];
if ( countryName ) {
const countryCallingCode = getCountryCallingCode( country );
acc.push(
toOption( country, countryCallingCode, countryName )
);
}
return acc;
}, [] );
}, [ countryNameDict ] );
}
Loading