-
Notifications
You must be signed in to change notification settings - Fork 909
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
Adds "Optimize with AI" buttons next to SEO assessments #21348
Adds "Optimize with AI" buttons next to SEO assessments #21348
Conversation
…ation-ai-buttons-next-to-seo-keyphrase-assessments
@FAMarfuaty Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
…ents' of github.com:Yoast/wordpress-seo into implementation-ai-buttons-next-to-seo-keyphrase-assessments
… is used to track whether there is an active AI Fixes button so we can display the button background color and the icon color differently when they are pressed. This is only relevant in Premium. In Free, this logic is irrelevant because clicking on the button will only display the Upsell modal.
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: Really well done! 👍 I've added a few small comments to potentially address. Feel free to ignore some of these.
ACC: I've added one question to the ATP that we need to discuss. I've taken it for a quick spin. It seems like everything is in order, just two comments:
- It should not be possible to have the AI button active as well as a highlight button. It would be great if they would cancel each other out.
- When clicking the eye button, the AI button moves up just a bit (1px?), and sometimes the assessment text moves up just a bit as well.
packages/js/src/ai-assessment-fixes/components/AIAssessmentFixesButton.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Martijn van der Klis <[email protected]>
… deactivate the AI Fixes button
… deactivate the highlighting button and remove the highlighting from the editor
…seo into implementation-ai-buttons-next-to-seo-keyphrase-assessments
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: Looks great! 😸
@mhkuu I pushed a commit to fix tiny typos in JSDoc 😸 |
const isPremium = getL10nObject().isPremium; | ||
|
||
// The reason of adding the check if Elementor is active or not is because `isBlockEditor` method also returns `true` for Elementor. | ||
return hasAIFixes && isBlockEditor() && ! this.props.isElementor && ( |
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.
This helper to check if Block editor is active isBlockEditor
helper packages/js/src/helpers/isBlockEditor.js
is not quite accurate: it also returns true
inside Elementor. We got the value from WP_Screen::get()->is_block_editor();
. As a workaround to make sure that the AI button is not shown in Elementor, here I added an extra check that the editor is NOT Elementor. So far I haven't found a more reliable method to check for Block editor. If such method exist, I guess that method would be preferable 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.
Hmm, it's not optimal -- I guess we could also consider removing
'isBlockEditor' => WP_Screen::get()->is_block_editor(), |
But I'm OK with the current implementation as well.
ACC: Everything works as advertised, there is just a small problem with the Elementor in-between-screen, but as the behaviour of the metabox/sidebar there is erratic anyway, we can have a look at that later. Nice work! 🚀 |
Context
Summary
This PR can be summarized in the following changelog entry:
AnalysisResult.js
.hasAIFixes
inAssessmentResult
class.hasAIFixes
totrue
for a certain condition for keyphrase in introduction, keyphrase in subheading, keyphrase density, and keyphrase distribution assessment's result.IconButtonBase
.IconAIFixesButton
.Relevant technical choices:
@yoast/components
package. This is because even though technically we can use@yoast/ui-library
package for this purpose, the stylings of the new AI button will not be consistent with the highlighting and the edit button. For the sake of consistency and UI cohesiveness, we are using the same stylings/components that we're using for the highlighting and the edit button. Eventually, in the new Tailwind style, we need to replace all the buttons and tooltips at once.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
WordPress
Shopify
12px
gapRelevant 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:
analysis-report
package which is also being used in Shopify. Please smoke test in shopify and confirm that we didn't change the behaviour/display of the eye icon.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 https://github.com/Yoast/wordpress-seo-premium/issues/4285