Skip to content

UX improvement on WordPress.com account connection #1068

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Oct 29, 2021
Merged

UX improvement on WordPress.com account connection #1068

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9fa7d17
Add wp-logo.svg.
ecgan Oct 27, 2021
4f82292
Add early test version of WPComAccountCard.
ecgan Oct 27, 2021
70ca60b
Move WPComAccountCard.
ecgan Oct 28, 2021
86705da
Code refactor with ConnectWPComAccountCard.
ecgan Oct 28, 2021
54d043a
Add ConnectedWPComAccountCard.
ecgan Oct 28, 2021
828c3cf
Code refactor with new component structure.
ecgan Oct 28, 2021
6ffab4f
Display ConnectedIconLabel in ConnectedWPComAccountCard.
ecgan Oct 28, 2021
c21b63d
Fix css linting.
ecgan Oct 28, 2021
c0e5b96
Remove unnecessary div in ConnectedIconLabel.
ecgan Oct 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions js/src/components/connected-icon-label/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* External dependencies
*/
import classnames from 'classnames';
import { __ } from '@wordpress/i18n';
import { Flex, FlexItem } from '@wordpress/components';
import GridiconCheckmarkCircle from 'gridicons/dist/checkmark-circle';

/**
* Internal dependencies
*/
import './index.scss';

const ConnectedIconLabel = ( props ) => {
const { className } = props;

return (
<div className={ classnames( 'gla-connected-icon-label', className ) }>
<Flex align="center" gap={ 1 }>
<FlexItem>
<GridiconCheckmarkCircle />
</FlexItem>
<FlexItem>
{ __( 'Connected', 'google-listings-and-ads' ) }
</FlexItem>
</Flex>
</div>
);
};

export default ConnectedIconLabel;
8 changes: 8 additions & 0 deletions js/src/components/connected-icon-label/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
.gla-connected-icon-label {
fill: currentColor;
color: #23a713;

svg {
display: block;
}
}
2 changes: 1 addition & 1 deletion js/src/setup-mc/setup-stepper/setup-accounts/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ const SetupAccounts = ( props ) => {
) }
>
<VerticalGapLayout size="large">
<WordPressDotComAccount />
<WordPressDotComAccount jetpack={ jetpack } />
<GoogleAccountCard disabled={ isGoogleAccountDisabled } />
</VerticalGapLayout>
</Section>
Expand Down
46 changes: 0 additions & 46 deletions js/src/setup-mc/setup-stepper/setup-accounts/wordpressdotcom-account/card-content.js
View file
Open in desktop

This file was deleted.

14 changes: 7 additions & 7 deletions ...ordpressdotcom-account/connect-account.js → ...com-account/connect-wpcom-account-card.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import AppButton from '.~/components/app-button';
import TitleButtonLayout from '.~/components/title-button-layout';
import useDispatchCoreNotices from '.~/hooks/useDispatchCoreNotices';
import useApiFetchCallback from '.~/hooks/useApiFetchCallback';
import WPComAccountCard from './wpcom-account-card';

const ConnectAccount = () => {
const ConnectWPComAccountCard = () => {
const { createNotice } = useDispatchCoreNotices();
const [ fetchJetpackConnect, { loading, data } ] = useApiFetchCallback( {
path: '/wc/gla/jetpack/connect',
Expand All @@ -33,12 +33,12 @@ const ConnectAccount = () => {
};

return (
<TitleButtonLayout
title={ __(
'Connect your WordPress.com account',
<WPComAccountCard
description={ __(
'Required to connect with Google',
'google-listings-and-ads'
) }
button={
indicator={
<AppButton
isSecondary
loading={ loading || data }
Expand All @@ -52,4 +52,4 @@ const ConnectAccount = () => {
);
};

export default ConnectAccount;
export default ConnectWPComAccountCard;
17 changes: 17 additions & 0 deletions ...p-mc/setup-stepper/setup-accounts/wordpressdotcom-account/connected-wpcom-account-card.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* Internal dependencies
*/
import getConnectedJetpackInfo from '.~/utils/getConnectedJetpackInfo';
import WPComAccountCard from './wpcom-account-card';
import ConnectedIconLabel from '.~/components/connected-icon-label';

const ConnectedWPComAccountCard = ( { jetpack } ) => {
return (
<WPComAccountCard
description={ getConnectedJetpackInfo( jetpack ) }
indicator={ <ConnectedIconLabel /> }
/>
);
};

export default ConnectedWPComAccountCard;
18 changes: 8 additions & 10 deletions js/src/setup-mc/setup-stepper/setup-accounts/wordpressdotcom-account/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
/**
* Internal dependencies
*/
import Section from '.~/wcdl/section';
import CardContent from './card-content';
import ConnectedWPComAccountCard from './connected-wpcom-account-card';
import ConnectWPComAccountCard from './connect-wpcom-account-card';

const WordPressDotComAccount = () => {
return (
<Section.Card>
<Section.Card.Body>
<CardContent />
</Section.Card.Body>
</Section.Card>
);
const WordPressDotComAccount = ( { jetpack } ) => {
if ( jetpack.active === 'yes' ) {
return <ConnectedWPComAccountCard jetpack={ jetpack } />;
}

return <ConnectWPComAccountCard />;
};

export default WordPressDotComAccount;
34 changes: 34 additions & 0 deletions ...setup-mc/setup-stepper/setup-accounts/wordpressdotcom-account/wpcom-account-card/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* External dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import AccountCard from '.~/components/account-card';
import wpLogoURL from './wp-logo.svg';

const WPComAccountCard = ( props ) => {
return (
<AccountCard
appearance={ {
icon: (
<img
src={ wpLogoURL }
alt={ __(
'WordPress.com Logo',
'google-listings-and-ads'
) }
width="40"
height="40"
/>
),
title: 'WordPress.com',
} }
Comment on lines +15 to +28
Copy link
Member

Choose a reason for hiding this comment

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

❔
Would you think it could be more consistent on code perspective to add these to APPEARANCE and appearanceDict within the AccountCard component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that. For the long term code perspective, I think it may not be a good idea. If we were to make it consistent to handle WordPress.com Logo SVG, the AccountCard component is going to get more bloated. For example, in this Setup MC Step 1 page, even though we are not displaying phone and store icon, when we use AccountCard, those icons are imported.

I prefer to use composition here, and keep AccountCard general purpose and lean. That would also help with file splitting and tree shaking when we want to go towards that direction.

Copy link
Member

Choose a reason for hiding this comment

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

If we were to make it consistent to handle WordPress.com Logo SVG, the AccountCard component is going to get more bloated. For example, in this Setup MC Step 1 page, even though we are not displaying phone and store icon, when we use AccountCard, those icons are imported.

The size ratios of phone and store icons including their dependencies in the index.js are highlighted by red background. (Total three blocks. The zoom-in of phone icon block.)

Their size is only about 1.3 kB after gzip. I think even if the <AccountCard> file is getting more bloated, the impact is not a primary concern compared to the overall file size for the present.

2021-11-01 16 58 46

That would also help with file splitting and tree shaking when we want to go towards that direction.

For the long term, regardless of their file size ratios, we could still achieve tree shaking and further file splitting by moving them to another file under the same folder of <AccountCard>:

// Another standalone file
export const APPEARANCE_PHONE = {
	icon: <GridiconPhone size={ 32 } />,
	title: __( 'Phone number', 'google-listings-and-ads' ),
};

export const APPEARANCE_ADDRESS = {
	icon: <Icon icon={ storeIcon } size={ 32 } />,
	title: __( 'Store address', 'google-listings-and-ads' ),
};

And expose them from <AccountCard>:

// .~/components/account-card/index.js
export { APPEARANCE_ADDRESS, APPEARANCE_PHONE } from './appearance-contact-info';

Then the bundle of consumer side would not contain phone and store icon codes if it doesn't use any of the constants.

// A consumer component depends on the account-card file
import AccountCard, { APPEARANCE_GOOGLE } from '.~/components/account-card';

Copy link
Contributor Author

@ecgan ecgan Nov 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The size ratios of phone and store icons including their dependencies in the index.js are highlighted by red background. (Total three blocks. The zoom-in of phone icon block.)

That's a good analysis 🙂 👍 What is that tool that you use to generate that treemap? 🤔

Their size is only about 1.3 kB after gzip.

Yes, the size for the phone and store icons is small and not a primary concern for the present.

To be clear, the mention of phone and store icons is to serve as an example. The point I was trying to bring up is that it can have a better code architecture design and be more scalable for the long run. With the current approach, if we are not mindful and careful, we might accidentally add a super large icon and it would be included in the bundle even though it is actually not used in the page.

For the long term, regardless of their file size ratios, we could still achieve tree shaking and further file splitting by moving them to another file under the same folder of <AccountCard>:

Yes, correct, and that's why I was doing this WPComAccountCard as a separate file (though not under the same folder of AccountCard), so that we don't need to do that later.

Then the bundle of consumer side would not contain phone and store icon codes if it doesn't use any of the constants.

The problem is, when we use <AccountCard>, phone and store icons are still included in the bundle, because of the use of appearanceDict (which has reference to the constants) within the <AccountCard> component:

google-listings-and-ads/js/src/components/account-card/index.js

Lines 82 to 85 in 454cd79

const { icon, title } =
typeof appearance === 'object'
? appearance
: appearanceDict[ appearance ];

google-listings-and-ads/js/src/components/account-card/index.js

Lines 29 to 49 in 454cd79

const appearanceDict = {
[ APPEARANCE.GOOGLE ]: {
icon: (
<img
src={ googleLogoURL }
alt={ __( 'Google Logo', 'google-listings-and-ads' ) }
width="40"
height="40"
/>
),
title: __( 'Google account', 'google-listings-and-ads' ),
},
[ APPEARANCE.PHONE ]: {
icon: <GridiconPhone size={ 32 } />,
title: __( 'Phone number', 'google-listings-and-ads' ),
},
[ APPEARANCE.ADDRESS ]: {
icon: <Icon icon={ storeIcon } size={ 32 } />,
title: __( 'Store address', 'google-listings-and-ads' ),
},
};

Edit: oops, I guess what you mean is that when we "move them to another file", we would also get rid of appearanceDict, and we would be using AccountCard like so, and that should be good and no problem: 😄

<AccountCard
  appearance={APPEARANCE_PHONE}
/>

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 replying.

I'm good with both code structures. Just to clarify that even if these assets are imported from <AccountCard>, bundle size, tree shaking, and code splitting will not be an issue.

What is that tool that you use to generate that treemap?

It's Webpack Bundle Analyzer. And a relevant usage can be found in the Detailed test instructions section in #1073.

With the current approach, if we are not mindful and careful, we might accidentally add a super large icon and it would be included in the bundle even though it is actually not used in the page.

I think with the current code structure, no matter where the huge inline SVG appears, it will be included in index.js at all. And I believe it will be discovered by the bundlewatch check.

{ ...props }
/>
);
};

export default WPComAccountCard;
5 changes: 5 additions & 0 deletions ...p-stepper/setup-accounts/wordpressdotcom-account/wpcom-account-card/wp-logo.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.