UX improvement on WordPress.com account connection #1068
Conversation
Remove old CardContent and ConnectAccount, and use new ConnectWPComAccountCard and ConnectedWPComAccountCard instead. Also simplify code by passing down jetpack as props to WordPressDotComAccount, so we don't need to use useJetpackAccount hook and check for isResolving, it was never called anyway because the check has been done in SetupAccounts.
| if ( isResolving ) { | ||
| return <AppSpinner />; | ||
| } | ||
|
|
||
| if ( ! jetpack ) { | ||
| return ( | ||
| <TitleButtonLayout | ||
| title={ __( | ||
| 'Error while loading WordPress.com account info', | ||
| 'google-listings-and-ads' | ||
| ) } | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Note: I believe we don't need this because it is already checked for in SetupAccounts parent component.
| appearance={ { | ||
| icon: ( | ||
| <img | ||
| src={ wpLogoURL } | ||
| alt={ __( | ||
| 'WordPress.com Logo', | ||
| 'google-listings-and-ads' | ||
| ) } | ||
| width="40" | ||
| height="40" | ||
| /> | ||
| ), | ||
| title: 'WordPress.com', | ||
| } } |
There was a problem hiding this comment.
❔
Would you think it could be more consistent on code perspective to add these to APPEARANCE and appearanceDict within the AccountCard component?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If we were to make it consistent to handle WordPress.com Logo SVG, the
AccountCardcomponent 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 useAccountCard, 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.
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';There was a problem hiding this comment.
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
google-listings-and-ads/js/src/components/account-card/index.js
Lines 29 to 49 in 454cd79
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}
/>
There was a problem hiding this comment.
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.
Changes proposed in this Pull Request:
Part of #1065 .
This PR changes the WordPress.com account connection card to the newer design. The account connection functionality and track events should all remain the same.
Screenshots:
To be connected:
Connected account:
Detailed test instructions:
Changelog entry