-
Notifications
You must be signed in to change notification settings - Fork 908
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
First run at a setup #21965
First run at a setup #21965
Conversation
Pull Request Test Coverage Report for Build 2e90e5baee9d8bd392c2a163eb41788468aea0ffWarning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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: 🏗️
Aside from the comments, I think there might be some changes in the architecture when we cross-reference the sitekit data with the indexable data, but let's tackle those with the next PR.
src/dashboard/user-interface/search-rankings/abstract-ranking-route.php
Outdated
Show resolved
Hide resolved
src/dashboard/user-interface/search-rankings/abstract-ranking-route.php
Outdated
Show resolved
Hide resolved
src/dashboard/infrastructure/search-console/search-console-parameters.php
Outdated
Show resolved
Hide resolved
src/dashboard/infrastructure/search-rankings/search-rankings-parser.php
Outdated
Show resolved
Hide resolved
src/dashboard/infrastructure/search-console/site-kit-search-console-adapter.php
Show resolved
Hide resolved
* | ||
* @var int | ||
*/ | ||
private $seo_score; |
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.
I'm having reservations about whether the seo_score
should be here or it should be injected by an indexable-related domain class, but let's see that in the final backend task for that 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.
I'm unresolving this because indeed it needs to be dealt with in the future, so lets keep it open for future reference.
There are going to be search-ranking data that will not have seo scores, so we will need a tweak of the approach here, which we can do when we finalize the endpoint,
src/dashboard/infrastructure/search-rankings/search-rankings-parser.php
Outdated
Show resolved
Hide resolved
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.
I want to re-iterate this point and actually extend it for the whole data provider class.
These classes being in such a generic domain that's about dashboard
in general, makes me feel weird about our already-implemented SEO/Readability score widgets that are also about dashboard
, are also repositories that deal with data/parameters/anything else that directory is about, but are being excluded by those classes because they are different things.
In any case, I dont want to block merging this PR because of this, we can talk about it and maybe iterate it on the next stage, when the indexable data come into play
So, CR + Acceptance test is ✅
class Search_Data implements Data_Interface { | ||
|
||
/** | ||
* The amount of clicks a `key` gets. |
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.
There's no longer a key
but rather a subject
. Don't want to block merging because of that, I'll correct in my subsequent PR if that's ok.
* | ||
* @var int | ||
*/ | ||
private $seo_score; |
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.
I'm unresolving this because indeed it needs to be dealt with in the future, so lets keep it open for future reference.
There are going to be search-ranking data that will not have seo scores, so we will need a tweak of the approach here, which we can do when we finalize the endpoint,
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
search-rankings-parser
class that does not do alot yet but as a blueprint where we can link the indexables to the api results.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
With the
YOAST_SEO_GOOGLE_SITE_KIT_FEATURE
feature flag enabled:112
.wp-json/yoast/v1/top_pages?limit=5
you should see an API response with your top 5 pages in terms of clicks. The response should look something like this:wp-json/yoast/v1/top_queries?limit=5
you should see an API response with your top 5 queries in terms of clicks. The response will look like something like the above but with queries instead of URLs in thesubject
attributes.With the
YOAST_SEO_GOOGLE_SITE_KIT_FEATURE
feature flag disabled: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 #