-
Notifications
You must be signed in to change notification settings - Fork 21
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
Free Listings + Paid Ads: Update learn more links, FAQs, CSS disclaimer, submission success modal, and title of free listings settings #1679
Conversation
… setup of onboarding flow
… side in disabled style.
… telling whether it's ready to continue the connection processing.
…up of onboarding flow
… the onboarding is completed along with paid ads
💅 In Figma the CSS Disclaimer texts is 11px and in implementation it's 12px |
💅 In disclaimer CSS --- I can still click on the elements (for example the links of the disclaimer) when it's disabled |
@@ -19,19 +19,14 @@ import GuidePageContent, { | |||
ContentLink, | |||
} from '.~/components/guide-page-content'; | |||
import AddPaidCampaignButton from '.~/components/paid-ads/add-paid-campaign-button'; | |||
import { GUIDE_NAMES, LOCAL_STORAGE_KEYS } from '.~/constants'; | |||
import { glaData, GUIDE_NAMES, LOCAL_STORAGE_KEYS } from '.~/constants'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 This glaData
could be in UPPERCASE because it's a constant. I'm not sure if this is coming from the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The glaData
references to window.glaData
, and its declaration and its lower-case properties are created by PHP side. In addition, it's more like preloading data rather than a collection of constants. Considering this seems to be out of the scope of this PR/project, I'm not leaning towards adjusting it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM
💅 Check if the copywriting and UI of CSS disclaimer --> Left some minor comments. Not really a blocker
✅ two "Learn more" links on the left side.
✅ Checked the FAQs
✅ Checked the Guide Modal with and without Ads Completed
💅 Would be cool to add a test at least for the Success Guide Modal behaviour with and without Ads Completed. But could be done in future PRs
Thanks for the code review, @puntope
Browsers may have a limitation to force the font size smaller than n to a minimum size when rendering, for example, Chrome uses 12px as the minimum size, so I didn't style it with 11px. Although the
I think it should be ok that those links are clickable in CSS disclaimer when it's rendered in disabled style. The prop names |
Changes proposed in this Pull Request:
This PR implements the rest parts of 📌 Update other UI changes in #1610.
Screenshots:
📷 Disclaimer of Comparison Shopping Service
📷 FAQs in the ads setup
📷 Submission success modal when the onboarding is completed along with paid ads
📷 Different title on the edit free listings page
Detailed test instructions:
isPreconditionReady
here tofalse
would be easier to check the UI state in the disabled style./wp-admin/admin.php?page=wc-admin&guide=submission-success&path=%2Fgoogle%2Fproduct-feed
.adsSetupComplete
value totrue
and refresh page to simulate the finished ads setup. Check if the modal has the same copywriting as it is on Figma.Changelog entry