-
Notifications
You must be signed in to change notification settings - Fork 383
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
Introduce Site Review to the Onboarding Wizard #6596
Conversation
6bf38d4
to
0dce63a
Compare
Plugin builds for ce6c456 are ready 🛎️!
|
Codecov Report
@@ Coverage Diff @@
## develop #6596 +/- ##
=============================================
+ Coverage 76.54% 76.61% +0.07%
- Complexity 6300 6313 +13
=============================================
Files 246 248 +2
Lines 19736 19796 +60
=============================================
+ Hits 15107 15167 +60
Misses 4629 4629
Flags with carried forward coverage won't be shown. Click here to find out more.
|
150976c
to
418eeda
Compare
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.
Looks great! Good work.
418eeda
to
2686f36
Compare
b7af591
to
ef7fed0
Compare
The list of Onboarding Wizard pages can be dynamically changed depending on the theme mode. The actual pages list is memoized in the `adaptedPages` array. The `adaptedPages` list should be used as a source of truth (after all, the `activePageIndex` references the `adaptedPages` array). In some cases the `activePageIndex` can get out of sync with the `adaptedPages` array. For instance, when viewing the last page and one of the previous pages gets removed the `activePageIndex` references a non-existent item. With this commit, we're stop keeping active page index in the state and instead track current page itself. Based on the `currentPage` object and the `adaptedPages` array we can determine the `activePageIndex`. This eliminates a risk of referencing a non-existent array item.
@westonruter I've addressed your feedback. Unfortunately, there's (again) an issue with a failing E2E test. I can look into that in several hours (tomorrow morning). |
Argh! It passed on a second try. |
Co-authored-by: Weston Ruter <[email protected]>
@westonruter I've addressed your feedback and also hopefully (I couldn't replicate the error locally) fixed that flaky E2E test. |
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.
Great work!
@westonruter Thanks for removing the redundant cap checks, improving the tests and pushing the PR through the finish line! |
Summary
The main update this PR brings is adding a Site Review feature to the last step of the Onboarding Wizard:
Along with the Site Review, a new panel has been added to the AMP Settings screen. It can be dismissed by a user (this gets stored in the user meta) and re-appears whenever a template mode is changed (after saving settings).
Besides adding the Site Review feature, this PR updates some of the microcopy and styles in the Onboarding Wizard (which partially covers #4719).
Note that the target branch for this PR is
feature/5578-add-new-settings
(#6501).Fixes #6071
Checklist