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

Fix issues and react warnings for verification code components #1007

Merged
merged 10 commits into from
Sep 15, 2021
13 changes: 9 additions & 4 deletions js/src/components/app-button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,20 @@ const AppButton = ( props ) => {
onClick( ...args );
};

const text = [];
const classes = [ 'app-button', className ];
let text;

if ( loading ) {
text.push( <Spinner /> );
text = <Spinner />;
}

if ( passedInText ) {
text.push( passedInText );
text = (
<>
{ loading && <Spinner /> }
{ passedInText }
</>
);

if ( rest.icon ) {
classes.push( 'app-button--icon-with-text' );
Expand All @@ -71,7 +76,7 @@ const AppButton = ( props ) => {
<Button
className={ classnames( ...classes ) }
disabled={ disabled || loading }
text={ text.length ? text : undefined }
text={ text }
onClick={ handleClick }
{ ...rest }
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import AccountCard, { APPEARANCE } from '.~/components/account-card';
import AppButton from '.~/components/app-button';
import AppSpinner from '.~/components/app-spinner';
import EditPhoneNumberContent from './edit-phone-number-content';
import VerifyPhoneNumberContent from './verify-phone-number-content';
import './phone-number-card.scss';

const noop = () => {};
Expand All @@ -22,6 +23,62 @@ const basePhoneNumberCardProps = {
appearance: APPEARANCE.PHONE,
};

// TODO: remove <DemoVerifyPhoneNumberCard> before merging PR.
export function DemoVerifyPhoneNumberCard() {
const display = '+1 213 373 4253';
const [ unverifiedPhoneNumber, setUnverifiedPhoneNumber ] = useState(
null
);

const cardContent = unverifiedPhoneNumber ? (
<>
<CardDivider />
<VerifyPhoneNumberContent
{ ...unverifiedPhoneNumber }
onPhoneNumberVerified={ () => {
// eslint-disable-next-line no-console
console.log( 'onPhoneNumberVerified' );
setUnverifiedPhoneNumber( null );
} }
/>
</>
) : null;

const indicator = (
<AppButton
isSecondary
text={ unverifiedPhoneNumber ? 'Edit' : 'Send verification code' }
onClick={ () => {
if ( unverifiedPhoneNumber ) {
setUnverifiedPhoneNumber( null );
} else {
setUnverifiedPhoneNumber( {
verificationMethod: 'SMS',
country: 'US',
number: '+12133734253',
display,
} );
}
} }
/>
);

return (
<section className="wcdl-section">
<header />
<div className="wcdl-section__body">
<AccountCard
{ ...basePhoneNumberCardProps }
description={ display }
indicator={ indicator }
>
{ cardContent }
</AccountCard>
</div>
</section>
);
}

/**
* @typedef { import(".~/hooks/useGoogleMCPhoneNumber").PhoneNumber } PhoneNumber
* @typedef { import("./edit-phone-number-content").onPhoneNumberChange } onPhoneNumberChange
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ export default function VerificationCodeControl( {
};

const handleKeyDown = ( e ) => {
const { dataset, selectionStart, value } = e.target;
const { dataset, selectionStart, selectionEnd, value } = e.target;
const idx = Number( dataset.idx );

switch ( e.keyCode ) {
case KEY_CODE_LEFT:
case KEY_CODE_BACKSPACE:
if ( selectionStart === 0 ) {
if ( selectionStart === 0 && selectionEnd === 0 ) {
maybeMoveFocus( idx - 1 );
}
break;
Expand Down Expand Up @@ -103,18 +103,38 @@ export default function VerificationCodeControl( {
}
};

useEffect( () => {
maybeMoveFocus( 0 );
}, [] );

useEffect( () => {
inputsRef.current.forEach( ( el ) => ( el.value = '' ) );
maybeMoveFocus( 0 );

setDigits( initDigits );
onCodeChangeRef.current( toCallbackData( initDigits ) );
}, [ resetNeedle ] );

/**
* Since the <InputControl> has an internal state management that always controls the actual `value` prop the <input>,
* the <InputControl> is forced to be a controlled input.
* When using it, it's always necessary to specify `value` prop from the below <AppInputControl>
* to avoid the warning - A component is changing an uncontrolled input to be controlled.
*
* @see https://github.com/WordPress/gutenberg/blob/%40wordpress/components%4012.0.8/packages/components/src/input-control/input-field.js#L47-L68
* @see https://github.com/WordPress/gutenberg/blob/%40wordpress/components%4012.0.8/packages/components/src/input-control/input-field.js#L115-L118
*
* But after specifying the `value` prop,
* the synchronization of external and internal `value` state will depend on whether the input is focused.
* It'd sync external to internal only if the input is not focused.
* So here we await the `digits` is reset back to `initDigits` by above useEffect and sync to internal value,
* then move the focus calling after the synchronization tick finished.
*
* @see https://github.com/WordPress/gutenberg/blob/%40wordpress/components%4012.0.8/packages/components/src/input-control/input-field.js#L73-L90
*
* And it's also used for the first time focus after this component being mounted.
*/
useEffect( () => {
if ( digits === initDigits ) {
maybeMoveFocus( 0 );
}
}, [ resetNeedle, digits ] );
Copy link
Member

Choose a reason for hiding this comment

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

👍 Thanks for the detailed explanations :) It makes it clear why the code is here not where it was before.
💅 However, I think we could also add a note of why we need it at all/what it does, for somebody who reads the file as is, not as a change to the previous state.

I made a branch with a small proposal fix/1002-1003-1004-verification-code...fix/1002-1003-1004-verification-code-comments, feel free to merge it if you like it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Merged. Thanks!


return (
<Flex
className="gla-verification-code-control"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { Notice, Flex } from '@wordpress/components';
/**
* Internal dependencies
*/
import useIsMounted from '.~/hooks/useIsMounted';
import useCountdown from './useCountdown';
import Section from '.~/wcdl/section';
import Subsection from '.~/wcdl/subsection';
Expand Down Expand Up @@ -108,6 +109,7 @@ export default function VerifyPhoneNumberContent( {
display,
onPhoneNumberVerified,
} ) {
const isMounted = useIsMounted();
const [ method, setMethod ] = useState( verificationMethod );
const { second, callCount, startCountdown } = useCountdown( method );
const [ verification, setVerification ] = useState( null );
Expand All @@ -134,10 +136,12 @@ export default function VerifyPhoneNumberContent( {
verificationIdRef.current[ method ] = id;
} )
.catch( () => {
startCountdown( 0 );
if ( isMounted() ) {
startCountdown( 0 );
}
// TODO: [full-contact-info] add error handling.
} );
}, [ country, number, method, startCountdown ] );
}, [ country, number, method, startCountdown, isMounted ] );

const handleVerifyClick = () => {
setError( null );
Expand All @@ -150,8 +154,10 @@ export default function VerifyPhoneNumberContent( {
} )
.catch( ( e ) => {
// TODO: [full-contact-info] align to the real error data structure.
setError( e );
setVerifying( false );
if ( isMounted() ) {
setError( e );
setVerifying( false );
}
} );
};

Expand Down
21 changes: 21 additions & 0 deletions js/src/hooks/useIsMounted.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* External dependencies
*/
import { useEffect, useCallback, useRef } from '@wordpress/element';

/**
* Returns a function to check whether the caller component is mounted or unmounted.
* Usually, it's used to avoid the warning - Can't perform a React state update on an unmounted component.
*
* @return {() => boolean} A function returns a boolean indicates the caller component is mounted or unmounted.
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
*/
export default function useIsMounted() {
Copy link
Member

Choose a reason for hiding this comment

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


Looks like native Node.isConnected. I'm surprised that React does not have it somewhat built-in and we have to hand-craft it? Do we do something not popular/standard here?

Copy link
Member Author

@eason9487 eason9487 Sep 15, 2021

Choose a reason for hiding this comment

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

I'm surprised that React does not have it somewhat built-in and we have to hand-craft it?

It had had a similar API isMounted, but was deprecated. https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html

Do we do something not popular/standard here?

It is difficult to say whether this practice is popular/standard or not because it's not really an issue or bug in most cases when seeing that warning.

useIsMounted is here mainly to avoid that scary warning appearing, but in fact there's no memory leak problem nor additional side effects.

The warning has quite a long background, mainly because some earlier global store/state management libraries based on the "Flux Architecture" had subscription and unsubscription mechanisms to be plugged in the class components. As an observer to get the global state changes, if a component forgets to unsubscribe after unmounted, it might let the global store keep the reference of the component itself or the method declared within the component, which may further lead to memory leaks.

However, with the implementation of something like react-redux, these subscription processes have been replaced by Higher-Order Components (HOC) and the memory leak problem is no longer common. After react hooks, these warnings are also almost false positives alarm. As it turns out, this warning will be adjusted in the new version of react, and we will no longer need useIsMounted then.

const mountedRef = useRef( false );
useEffect( () => {
mountedRef.current = true;
return () => {
mountedRef.current = false;
};
}, [] );
return useCallback( () => mountedRef.current, [] );
}
2 changes: 2 additions & 0 deletions js/src/settings/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import DisconnectAccounts from './disconnect-accounts';
import ReconnectAccounts from './reconnect-accounts';
import EditContactInformation from './edit-contact-information';
import './index.scss';
import { DemoVerifyPhoneNumberCard } from '.~/components/contact-information/phone-number-card/phone-number-card';

const Settings = () => {
const { subpath } = getQuery();
Expand Down Expand Up @@ -42,6 +43,7 @@ const Settings = () => {
return (
<div className="gla-settings">
<NavigationClassic />
<DemoVerifyPhoneNumberCard />
<DisconnectAccounts />
<ContactInformationPreview />
</div>
Expand Down