Skip to content

Pre-processing of Card UI to reduce the future work conflicts between partial auth and UX improvements on onboarding page #1052

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

Merged
merged 12 commits into from
Oct 21, 2021

Conversation

eason9487
Copy link
Member

Changes proposed in this Pull Request:

Part of #1000

  • Add disabled and alignIcon props to <AccountCard> for styling
  • Replace the implementations within <GoogleAccountCard> by <AccountCard>
  • Merge the Cards of WordPress (JetPack) account and Google account into the same Section
  • Move the applying of disabled style down to the Card layer in the <GoogleMCAccount>
  • Remove the dependency of <GoogleAccount> within js/src/setup-ads/ads-stepper/setup-accounts/index.js

The requirements of UX improvements can be found at pcTzPl-6u

Additional notes

  • Related discussion Proposal/RFC convention that mimics CSS parts and state #1030 hasn't resulted in a conclusion. But eventually, we need more custom styles in the <AccountCard>, so I put the class names onto some internal elements in advance by the existed naming convention in the codebase. Such as the usages in the app-tab-nav
  • The main goal of this PR is to reduce the chance of conflict between the two projects. There will be some temporary inconsistency with other components on the page, which will be adjusted later in the subsequent development.
  • Some inconsistencies between two projects' Figma and the current codebase were confirmed with Jill:
    1. The permission description of Google account card should use the UX improvements version
    2. The title of Google account card should be "Google account", which is the same title on the re-connection page

Screenshots:

💡 The adjusted UI of WordPress and Google account cards is upon the red line, and the one under red line is the original UI.

📷 . Disabled card/secion

2021-10-18 16 34 19

📷 . Not yet connected

2021-10-18 16 35 29

📷 . Connected

2021-10-18 16 36 41

Detailed test instructions:

  1. Go to the onboarding page.
  2. The function of connecting Google account should work as before.
  3. Although it may not be easy to evaluate because I can't fully describe my later implementation, and I don't know the implementation design that Gan will take. Nevertheless, it should be expected that it would be easier to continue development of both projects and less conflict chance with this adjustment. 🤞

Changelog entry

@eason9487 eason9487 requested a review from a team October 18, 2021 11:28
@eason9487 eason9487 self-assigned this Oct 18, 2021
Comment on lines -26 to -35
if ( ! google ) {
return (
<TitleButtonLayout
title={ __(
'Error while loading Google account info',
'google-listings-and-ads'
) }
/>
);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't move this part to the new implementation, because with the previous continued development, it has been no longer entering this logic.

Comment on lines 10 to 12
&:not(:empty) {
margin-bottom: 8px;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

To remove the extra 8px height when rendering the loading spinner only at

if ( ! hasFinishedResolution ) {
return <AccountCard appearance={ {} } description={ <AppSpinner /> } />;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

💅 📜 I'm not sure if we discussed it already, but if we'd make Cards body a display: flex; flex-direction: column; gap: 8px; we would not have to bother about :not(:empty) and :last-child manual margin juggling, but would get that collapsing for empty and edge items for free.

If we are about to stay with manual reset, I'd add a code comment that it's to collapse the margin for empty titles. So we won't forget this in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

A similar discussion is here #1019 (comment).

In the <Subsection.Title> case, currently its parent elements are:

  • <Section.Card.Body>
  • <FlexBlock>
  • <Subsection>
  • <ContentButtonLayout>
  • <div>

If transfer the space style to upper layers, the changing scope will be much larger. I lean toward not doing so for now. Added comment by a41336c

<AppDocumentationLink
context="setup-mc-accounts"
linkId="required-google-permissions"
href="https://docs.woocommerce.com/document/google-listings-and-ads/#required-google-permissions"
Copy link
Member Author

Choose a reason for hiding this comment

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

The anchor should work after finishing #1053

appearance={ APPEARANCE.GOOGLE }
disabled={ disabled }
hideIcon
alignIcon="top"
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ 📜
Do we need alignIcon when it's hideIcon.
Also maybe we can somehow marge those two props together, to avoid such confusion.
Like, iconPosition = top | center | hidden?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for not stating it clearly first. 🙏 By the UX Improvements project, all account cards will have a leading icon. But I only adjusted Google account card for the Partial oauth project, and other account cards stay with the original visual results. To make it consistent as possible, I was thinking of hiding the icon here, and Gan can just remove all hideIcon when he is working on UX Improvements.

// cc @ecgan

const { google, hasFinishedResolution } = useGoogleAccount();

if ( ! hasFinishedResolution ) {
return <AccountCard appearance={ {} } description={ <AppSpinner /> } />;
Copy link
Contributor

Choose a reason for hiding this comment

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

📜 It wasn't here before, but we could consider making this element A11y friendly with indicating loading state

Copy link
Contributor

Choose a reason for hiding this comment

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

💅 📜 Also, maybe we can consider introducing a default behavior for no appearance, so we would not have to provide an empty object here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the UX Improvements project will have more interactive adjustments to all account cards on the onboarding page, and I don't know how Gan will do the design. So I don't aim to make detailed changes in this PR, but to stick to the original implementation as much as possible. And we could leave this 📜 as a dev note for Gan. 😎

// cc @ecgan

Copy link
Contributor

@tomalec tomalec left a comment

Choose a reason for hiding this comment

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

Reviewed the code, tested locally, the MC setup, Ads setup, and settings cards. LGTM besides one note about no-longer-needed code left, plus a number of cosmetic ideas.

Copy link
Contributor

@tomalec tomalec left a comment

Choose a reason for hiding this comment

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

pre-approving, as I guess there are still PRs planned that may address all remaining issues.

@eason9487 eason9487 merged commit 328faee into develop Oct 21, 2021
@eason9487 eason9487 deleted the tweak/1000-preprocess-card-ui branch October 21, 2021 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants