-
Notifications
You must be signed in to change notification settings - Fork 907
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
Create frontend providers and widget factory #22008
base: trunk
Are you sure you want to change the base?
Create frontend providers and widget factory #22008
Conversation
Initial setup for architecture change * move data into data provider * use widget factory to create score widgets
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.
Renamed and moved into the top-pages-widget
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.
Seems like a friendly hook for more data/error/isPending widgets. However, I'm not sure how caching/debouncing should be incorperated yet.
/** | ||
* @returns {WidgetTypeInfo[]} | ||
*/ | ||
static get widgetTypes() { | ||
return [ | ||
{ type: "seoScores" }, | ||
{ type: "readabilityScores" }, | ||
{ type: "topPages" }, | ||
]; | ||
} |
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.
Maybe remove? As it is not needed yet, if ever
@@ -75,11 +83,10 @@ domReady( () => { | |||
element={ | |||
<SidebarLayout> | |||
<Dashboard | |||
contentTypes={ contentTypes } | |||
widgetFactory={ widgetFactory } | |||
initialWidgets={ [ "seoScores", "readabilityScores", "topPages" ] } |
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.
Need feature flag check before adding topPages
const removeWidget = useCallback( ( type ) => { | ||
setWidgets( ( currentWidgets ) => currentWidgets.filter( ( widget ) => widget.type !== type ) ); | ||
}, [] ); | ||
|
||
return ( | ||
<> | ||
<PageTitle userName={ userName } features={ features } links={ 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.
Should this be a widget too?
* @returns {JSX.Element|null} The widget or null. | ||
*/ | ||
// eslint-disable-next-line no-unused-vars | ||
createWidget( widget, onRemove ) { |
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.
onRemove
is not yet used. Should be under the widget menu eventually, right?
Probably nice to have some RemoveWidget base that handles this in the future?
createWidget( widget, onRemove ) { | ||
switch ( widget.type ) { | ||
case "seoScores": | ||
if ( ! this.#dataProvider.hasFeature( "indexables" ) && this.#dataProvider.hasFeature( "seoAnalysis" ) ) { |
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 this move into the Widget? And is it worth an Widget layer if more need this?
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:
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 #