Streamline onboarding: Move FAQs to the bottom of pages #2531
Conversation
asvinb
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/2458-streamline-onboarding #2531 +/- ##
====================================================================
- Coverage 63.8% 63.8% -0.0%
====================================================================
Files 326 327 +1
Lines 5086 5090 +4
Branches 1229 1232 +3
====================================================================
+ Hits 3247 3248 +1
- Misses 1672 1674 +2
- Partials 167 168 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Left a few suggestions. Additionally, regarding your observation about the excessive space due to the MC card text, there is also extra space when the card is expanded to show the "Select an existing account" dropdown.
It looks like these styles that used to ensure that there was space between this card and the FAQ panel can be removed. That won't totally solve the issue you noted, but will help in some cases.
| @@ -47,15 +47,15 @@ const FormContent = ( { | |||
| <ConditionalSection show={ shouldDisplayTaxRate }> | |||
| <TaxRate /> | |||
| </ConditionalSection> | |||
| <StepContentFooter> | |||
| <StepContentActions> | |||
There was a problem hiding this comment.
Now that you've separated the StepContentActions from the StepContentFooter, I think it would be good to be consistent about wrapping StepContentActions in a StepContentsFooter component everywhere it's used, rather than just when it's in a Section with a FaqPanel. What do you think?
| <FaqsPanel context="campaign-management" faqItems={ faqItems } /> | ||
| </Section> | ||
| ); | ||
| return <FaqsPanel context="campaign-management" faqItems={ faqItems } />; |
There was a problem hiding this comment.
Now that you're removing the Section component from this and the paid-ads/faqs-section.js I think it would make more sense to rename them as well. Perhaps using an even a more specific name like AssetGroupFaqsPanel and PaidAdsFaqsPanel, respectively so their not confused with the primitive FaqsPanel component they consume.
There was a problem hiding this comment.
I was thinking of that! 👍
|
@asvinb I made some updates to this PR, per my feedback. Would you mind giving it a look before sending to QA? |
|
All good @joemcgill ! |
joemcgill
dismissed
their stale review
Changes requested have been addressed, but I worked on that code so am going to defer approving.
QA/Test Report- ✅Testing Environment -
Steps to Test- Please follow the steps mentioned in PR description and watch below attached screencast for all screens. Test Results - Tested the changes in the below mentioned browsers and all screens are showing the FAQ section below of CTA button.
Functional Demo / Screencast - Recording.799.mp4Next Step- Ready to Code Review(Woo) Plugin file with this fix-specific branch: Branch: feature/2488-move-faq |
joemcgill
changed the base branch from
develop
to
feature/2458-streamline-onboarding
|
@eason9487 this one is ready for your review |
eason9487
added
the
changelog: tweak
There was a problem hiding this comment.
Hi @asvinb! Thanks for working on this, I left a comment related to that FAQ section is not shown in the step 4 of the onboarding.
Regards that big gap on the step 1, from my opinion looks a bit odd to see so much space and the button looks a bit lost in the screen, not sure what would be better here but maybe we can ask for help from a designer to improve this part and we could work on this in a follow-up PR. WDYT?
| </StepContentFooter> | ||
| </StepContent> | ||
| ); | ||
| } | ||
|
|
||
| <PaidAdsFaqsPanel />; |
There was a problem hiding this comment.
It looks like this should be placed inside StepContentFooter component, otherwise the FAQ will be not displayed.
There was a problem hiding this comment.
Thanks for catching that @jorgemd24 . PR Updated!
|
@jorgemd24 @joemcgill One potential solution for the excessive spacing is moving the CTA and FAQ panel for the accounts setup step within the <Section
className="gla-google-mc-account-section"
description={ <GoogleMCDisclaimer /> }
disabledLeft={ ! isGMCPreconditionReady }
>
<GoogleMCAccountCard />
<VerticalGapLayout size="xlarge">
<StepContentActions>
<AppButton
isPrimary
disabled={ isContinueButtonDisabled }
onClick={ onContinue }
>
{ __( 'Continue', 'google-listings-and-ads' ) }
</AppButton>
</StepContentActions>
<Faqs />
</VerticalGapLayout>
</Section>It'll look like this: The only downside is that we are not using the |
There was a problem hiding this comment.
Thanks for the adjustments, LGTM 👍
One potential solution for the excessive spacing is moving the CTA and FAQ panel for the accounts setup step within the gla-google-mc-account-section section.
Visually, I think it looks better this way. We could also just move the CTA button and keep the FAQ in the footer.
<Section
className="gla-google-mc-account-section"
description={ <GoogleMCDisclaimer /> }
disabledLeft={ ! isGMCPreconditionReady }
>
<GoogleMCAccountCard />
<VerticalGapLayout size="xlarge">
<StepContentActions>
<AppButton
isPrimary
disabled={ isContinueButtonDisabled }
onClick={ onContinue }
>
{ __( 'Continue', 'google-listings-and-ads' ) }
</AppButton>
</StepContentActions>
</VerticalGapLayout>
</Section>
<StepContentFooter>
<Faqs />
</StepContentFooter>
I'm approving this PR so we can move forward, and we can discuss this in a separate issue or PR so we don't block this PR.
|
Thanks @asvinb the updates in
Given that we're reworking the account cards in #2509, let's hold off on making any additional changes. If we decide that we want to ship this FAQ panel change sooner, than doing something like @jorgemd24 suggests in #2531 (review) could be a good short-term solution. I'm going to go ahead and merge this into the feature branch for now. Thanks all! |
eason9487
changed the title
Changes proposed in this Pull Request:
Closes #2488
StepContentActionscomponent which replacesStepContentFooter.StepContentFootercomponent which uses the existingSectionandVerticalGapLayoutcomponents.xlargesize toVerticalGapLayoutthat uses the--large-gapCSS property for spacing.Screenshots:
Onboarding flow:
Editing a paid campaign
Optimizing your campaign
Detailed test instructions:
--large-gapCSS property as per the AC.Additional details:
On the first step of the onboarding process, there is extra space between the CTA and the section above because of the long text as per the screenshot:
Changelog entry