Add schema feature toggle and settings page#22772
Conversation
leonidasmi
added
the
changelog: non-user-facing
leonidasmi
added
changelog: enhancement
…k and FeatureCard for confirmation before disabling schema features
… and rename state variable
…-toggle-for-the-yoast-schema-graph
…oved responsiveness
…tions page with schema framework checks
…Lint complexity warning
…nd features pages
|
One thing I noticed, when schema is disabled via the filter: We then show in the integration page a message about the schema being disabled (rightly so), but then instruct the user to enable it in the settings, which is not right since they will not be able to enable it from there, until they remove the filter. So, to recap. When we have disabled schema via filter, we show this in the integrations page: |
|
Also, I'm not able to tell by the design, but when we have disabled schema via the filter, it looks like the main feature toggle is disabled in the schema page, but in the PR it's not: I've noticed that instead it has the same behavior with the main feature card (that is, it's not disabled but displays a warning modal when tried to be toggled on), so if we have double checked that it's ok with Tom, then all goo |
leonidasmi
left a comment
leonidasmi
left a comment
There was a problem hiding this comment.
CR: 🏗️
And some comments about the functionality above
| */ | ||
| public static function get_conditionals() { | ||
| return [ | ||
| Front_End_Conditional::class, |
There was a problem hiding this comment.
Come to think about it, this doesn't need the Front_End_Conditional because then it wouldnt work on the REST API.
Basically, if you have disabled schema via the option and visit a post's REST address (eg. http://example.com/wp-json/wp/v2/posts/<POST_ID> and search for "schema", you will find the schema even though you shouldn't.
We can remove that conditional and do nothing else, or create a conditional that checks if we're in REST request, but I'm leaning towards the former.
| * | ||
| * @return array<string, array<string, bool|string>> The schema API integrations status. | ||
| */ | ||
| private function get_schema_api_integrations(): array { |
There was a problem hiding this comment.
Everything in this function is duplicated code from Yoast\WP\SEO\Integrations\Admin::Integrations_Page basically, let's have a single source of truth.
Best to do is:
- Take what the
Integrations_Pageclass has and move it to the Schema_Configuration class (probably what you have already done here, but let's make sure that it's exactly the same data) - Make a dependency in the integrations page for the schema configuration class and use the latter's method there too
- That way, we have the single source of truth (the method of the schema config class) that's being used both in the integrations page class and the settings integrations class
|
|
||
| // Flag to check if the Schema Framework is enabled. | ||
| // eslint-disable-next-line dot-notation | ||
| const isSchemaFrameworkEnabled = Boolean( window.wpseoIntegrationsData[ "schema_framework_enabled" ] ); |
There was a problem hiding this comment.
We have multiple usages of this very same line, maybe there's a more graceful way to pass that information along the component tree? We can consider React's context for this?
There was a problem hiding this comment.
There are only 2 usages. A context makes sense if this pattern would be used in more places, which I doubt is the case.
| const [ isConfirmModalOpen, setIsConfirmModalOpen ] = useState( false ); | ||
| const [ isProgrammaticallyDisabledModalOpen, setIsProgrammaticallyDisabledModalOpen ] = useState( false ); | ||
|
|
||
| const handleToggleChange = useCallback( ( newValue ) => { |
There was a problem hiding this comment.
Again, we use this here and in the schema framework route, so let's have it centralized somewhere in a higher level and use the centralized version in both places
| const structuredDataLearnMoreLink = useSelectSettings( "selectLink", [], "https://yoast.com/features/structured-data/" ); | ||
| const learnMoreFilterLink = useSelectSettings( "selectLink", [], "https://yoa.st/schema-framework-filters" ); | ||
| const schemaApiLink = useSelectSettings( "selectLink", [], "https://yoa.st/schema-api" ); | ||
| const schemaDocumentationLink = useSelectSettings( "selectLink", [], "https://developer.yoast.com/features/schema/api/" ); |
There was a problem hiding this comment.
I think we want this link behind a shortlink as well? Best to check with Manuel for this, to confirm (same with the other link to our dev docs)
There was a problem hiding this comment.
Correct, this should be a shortlink. I've already asked for them.
Good appreciation 👍🏼 I'll double check it with @ux-tom Update:
|
I know what you mean, but in the design the toogle has always an Regarding your point of only showing the info modal in the Site Features toggle, and not in the Schema page toggle, I'm not sure whether it makes sense 🤔 So good that you notice it! I'll check it with @ux-tom as well! Update:
|
…erce dependency and enhance schema API integration checks
leonidasmi
left a comment
leonidasmi
left a comment
There was a problem hiding this comment.
CR: ✅, aside from one thing I missed and want us to improve:
- In
Schema_Disabled_Conditional, turnreturn ! $this->options->get( 'enable_schema', true );intoreturn $this->options->get( 'enable_schema', true ) === false;, to check for false specifically instead of just false-y values - I did that and performed some regression test and looks good, but before pushing that change, I wouldnt mind a second look from you too @JorPV
Acceptance: 🚧
A thing I noticed that I think we should handle better:
- Disable the schema via its setting
- Then add the filter that disables the schema via that hook:
add_filter( 'wpseo_json_ld_output', '__return_false' ); - Go to the schema settings page and you will see the toggle being turned off
- Toggle it on
- At that point, I would expect the modal that informs me that we can't turn this on because we have the filter enabled
- At the very least, even if I dont get that modal, I shouldn't be able to toggle that thing on
- Save the changes, which is done successfully so the user might think that they indeed turned on the schema feature
- Refresh the page and see that the schema is disabled! (this time you also see the alert informing that the filter is there)
You are right, |
|
@JorPV Things I've pushed myself and would appreciate a CR and regression test:
|
vraja-pro
left a comment
vraja-pro
left a comment
There was a problem hiding this comment.
Comments about site-features.js
| label={ __( "Enable feature", "wordpress-seo" ) } | ||
| disabled={ isDisabled } | ||
| checked={ disabledSetting === "language" || showProgrammaticallyDisabledModal ? false : value } | ||
| onChange={ handleToggleChange } |
There was a problem hiding this comment.
What about adding onChange to FormikValueChangeField to override the handleChange?
There was a problem hiding this comment.
After my explanation here, I dont see the benefit in that.
There was a problem hiding this comment.
As per a talk with @vraja-pro , we can ignore this for now and we can improve this after we merge both affected PRs.
| { showProgrammaticallyDisabledModal && programmaticallyDisabledModal( { | ||
| isOpen: isProgrammaticallyDisabledModalOpen, | ||
| onClose: handleProgrammaticallyDisabledModalClose, | ||
| } ) } |
There was a problem hiding this comment.
As per a talk with @vraja-pro , we can ignore this for now and we can improve this after we merge both affected PRs.
| const showConfirmationModal = Boolean( disableConfirmationModal ); | ||
| const showProgrammaticallyDisabledModal = Boolean( programmaticallyDisabledModal ); | ||
|
|
||
| const handleToggleChange = useSchemaToggleHandler( { |
There was a problem hiding this comment.
This is coupling the feature card to the schema framwork feature, better to have that logic in a separate component and pass that as children. We can overcome the toggle callback by adding a prop onChange or onEnable and override the FormikValueChangeField onChange.
There was a problem hiding this comment.
This is coupling the feature card to the schema framwork feature
Actually, this is NOT coupling the feature card to the schema framework, my last commit did what it needed to be done to avoid that.
Granted, the name of the toggle handler does carry a schema-related context, but that's the only thing that is still schema related. I'm pushing a commit to remedy that.
There was a problem hiding this comment.
Just pushed this, so I believe we have now completely decoupled the feature card component from the schema framework.
There was a problem hiding this comment.
As per a talk with @vraja-pro , we can ignore this for now and we can improve this after we merge both affected PRs.
| { showProgrammaticallyDisabledModal && programmaticallyDisabledModal( { | ||
| isOpen: isProgrammaticallyDisabledModalOpen, | ||
| onClose: handleProgrammaticallyDisabledModalClose, | ||
| } ) } |
There was a problem hiding this comment.
As per a talk with @vraja-pro , we can ignore this for now and we can improve this after we merge both affected PRs.
Agree, since the option shouldn't be considered disabled when it might be an empty string or other falsy value.
👍🏼 Only add the filter on frontend pages, not on admin.
🧠 good thinking! It makes sense to pass the modal as a prop. CR: ✅ |
|
Acceptance: ✅ (as per this thread) |
4 participants
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
const isSchemaFrameworkEnabled = Boolean( window.wpseoIntegrationsData[ "schema_framework_enabled" ] );flag to tell the Woocommerce SEO Integration component that this instance is being used in the Schema API integrations section.FormikValueChangeFieldto be able to intercept the "disable" action and show the confirmation modal before the value changes.enable_schemafrom optionsinc/options/class-wpseo-option-wpseo.phpis controlled by the user toggle whileisSchemaDisabledProgrammaticallycompute at runtime from the wpseo_json_ld_output filter in the theme functions.php.get_schema_api_integrations()Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Note
At the moment of writting these test instructions, the shortlinks have not been added yet (see thread 🧵 ). Therefore, some links will still re-direct to the yoast.com homepage.
Site features
Learn morelink should redirect in a new tab.Cancelbuttons and confirm that both close the modal and that the Shema Framework is still enabled.Turn off Schema Frameworkbutton.Discard changesbutton andYes, discard changesin the modal and confirm that the Schema is again enabled with the toggle and the icon with color.Schema page
Test feature toggle when disabled
http://example/wp-json/wp/v2/posts/<POST_ID>), you will see the schema not being removed fromyoast_head_json, although it SHOULD be removed fromyoast_headIntegrations page
Disable the Schema programmatically
functions.phpfile of your wp test site theme. (e.g. onewordpresstest/app/public/wp-content/themes/twentytwentyfour/functions.php)Learn more about the filterlink in the alert and confirm that it redirects to the To disable Schema entirely page in a new tab.Regression test schema
wpseo_json_ld_outputfilter and confirm that you don't see schema in the frontendTest feature toggle when disabledare true here too.WP SEO Structured Data SchemapluginWP SEO Schema->Settingsand check the YOAST SEO Default Schema JSON-LD disable optionTest case when schema is disabled both with the setting and the filter
add_filter( 'wpseo_json_ld_output', '__return_false' );Schema output disabledalert that informs users that the filter is activeRelevant 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:
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.Documentation
Quality assurance
grunt build:imagesand commited the results, if my PR introduces new images or SVGs.Innovation
innovationlabel.Fixes #826