-
Notifications
You must be signed in to change notification settings - Fork 874
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
21318 refactor seo data provision to script data #21339
base: trunk
Are you sure you want to change the base?
Conversation
…-data # Conflicts: # composer.json
Pull Request Test Coverage Report for Build 6ebbfba6f3acb97e17911989012bcf63226e2de5Details
💛 - 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 🏗️
Looking nice! 🥳
Mostly naming questions and then some PHPStorm type declaration stuff
Missing test coverage though? Tests that you added also don't cover the constructors
/** | ||
* The term. | ||
* | ||
* @param WP_Post $post The post. | ||
* | ||
* @return void | ||
*/ | ||
public function set_post( WP_Post $post ) { | ||
$this->post = $post; | ||
} |
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 was wondering why you would cache the post/term like this inside the repository.
Wouldn't it be nicer to just pass it to the get_seo_data
method?
} | ||
|
||
/** | ||
* The term. |
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 term. | |
* The post. |
src/editors/domain/seo/keywords.php
Outdated
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.
We call them keyphrases
in the UI nowadays. Though I'm pretty sure we have more occurences of keyword
in the code still. I do think we started moving towards the term keyphrase
? Maybe ask team Lingo.
Of course influencing all the naming here 😅
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.
In the decoupling issue, @vraja-pro named it SEO description or just description inside the SEO context here.
The meta part refers to the output tag on the frontend.
In other words, my vote would go to referring to description
and title
within this SEO domain context instead of prefixing them with meta-
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.
Same keyword vs keyphrase comment.
tests/Unit/Editors/Framework/Seo/Posts/Keywords_Data_Provider_Test.php
Outdated
Show resolved
Hide resolved
* | ||
* @return bool Whether the social templates should be used. | ||
*/ | ||
public function use_social_templates() { |
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 we need to deprecate this public function?
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.
Indeed, good catch
'isInsightsEnabled' => $this->is_insights_enabled(), | ||
]; | ||
|
||
$values = ( $values_to_set + $values ); | ||
$values = ( $repo->get_seo_data() + $values ); |
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.
Can we write like this?:
$values = ( $values_to_set + $values + $repo->get_seo_data() );
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 think we can, but I am kinda afraid to do so since it kind of seems confusing to me
use Yoast\WP\SEO\Tests\Unit\TestCase; | ||
|
||
/** | ||
* Class Keywords_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.
Typo
* Class Keywords_Test | |
* Class Description_Test |
use Yoast\WP\SEO\Tests\Unit\TestCase; | ||
|
||
/** | ||
* Class Keywords_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.
* Class Keywords_Test | |
* Class Keyphrase_Test |
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:
Do the following for both posts and terms.
Keyword usage:
the keyphrase
to it. Note down the post id.the keyphrase
to it aswell.wpseoScriptData.metabox.keyword_usage
Make sure the first post is there. Also addwpseoScriptData.metabox.keyword_usage_post_types
and make sure it says your keyphrase and your post type in there.Meta description:
wpseoScriptData.metabox.metadesc_template
make sure the value is the same as your meta description.wpseoScriptData.metabox.metaDescriptionDate
and make sure it is either the current date or the date your post got changed the last time.wpseoScriptData.metabox.metadesc_template
changes accordingly. andwpseoScriptData.metabox.metaDescriptionDate
is updated to the current dateMeta title:
wpseoScriptData.metabox.title_template
make sure the value is the same as your meta title. With the replacement variables unparsed.wpseoScriptData.metabox.title_template_no_fallback
and make sure it is the same.wpseoScriptData.metabox.title_template
is not changed.Social:
First content image:
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 #