-
Notifications
You must be signed in to change notification settings - Fork 913
493 add navigation sidebar with alert center and first time configuration links #21614
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
493 add navigation sidebar with alert center and first time configuration links #21614
Conversation
…github.com:Yoast/wordpress-seo into 493-add-navigation-sidebar-with-alert-center-and-first-time-configuration-links
Don't override the SidebarNavigation from the UI library (weird bugs came to life, maybe import order - not sure) But there is not need to override it, just using a separate component that wraps the original is fine
…avigation-sidebar-with-alert-center-and-first-time-configuration-links # Conflicts: # admin/class-config.php # packages/js/src/settings/app.js
Pull Request Test Coverage Report for Build 6d7fd296b04193b6f38c3e4dd05c9de9db7de4eeDetails
💛 - Coveralls |
…avigation-sidebar-with-alert-center-and-first-time-configuration-links
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.
CR && AT ✅
I have some remarks, but no blockers!
<LiveAnnouncer> | ||
<LiveMessage message={ ariaLiveTitle } aria-live="polite" /> | ||
<Helmet> | ||
<title>Dashboard</title> |
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.
Should that be title
or something Dashboard - route name
?
*/ | ||
const Content = () => { | ||
return <> | ||
<header className="yst-p-8 yst-border-b yst-border-slate-200"> |
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.
Why aren't you using the RouteLayout
component here?
@@ -1,5 +1,4 @@ | |||
#wpseo-first-time-configuration { | |||
.yst-root { | |||
#yoast-configuration { |
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.
Why is this renamed? 🤔
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Dashboard new
.define( 'YOAST_SEO_NEW_DASHBOARD_UI', true );
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #