-
Notifications
You must be signed in to change notification settings - Fork 902
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 ui placeholders for selecting publishing principles #20349
Conversation
Pull Request Test Coverage Report for Build 5210243519
💛 - 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 below, I also noticed:
There's no way to unselect an option. Meaning, if a page is selected for a policy, there's no way to clear that (only via the Reset Options, in the Test Helper)
I think we need to tackle that too :)
@@ -55,7 +63,7 @@ | |||
.yst-autocomplete__options { | |||
@apply | |||
yst-absolute | |||
yst-z-10 | |||
yst-z-20 |
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.
Is this expected to potentially change any UI behavior of other parts of the plugins or Shopify? If so, they need to be added in the Impact check and/or inform the Shopify team
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.
Yes this component is only used here and in the user picker, as well in shopifiy. and there is no change in existing UI :) Since this was only needed when this component is all the way at the bottom of the screen.
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 seems to be more an issue with our save footer having the same Z value of 10. But I suppose it makes sense to move our UI library to a step higher for the "popup" things. Maybe good to create an issue for the modal and select too then.
However, you should add a changelog entry for this then. Changing the style is a user-facing thing for the UI library.
packages/js/src/settings/components/formik-page-select-field.js
Outdated
Show resolved
Hide resolved
packages/js/src/settings/components/formik-page-select-field.js
Outdated
Show resolved
Hide resolved
packages/js/src/settings/components/formik-page-select-field.js
Outdated
Show resolved
Hide resolved
$policies = $this->maybe_add_policy( $policies, $settings['wpseo_titles']['diversity_policy_id'], 'diversity_policy_id' ); | ||
$policies = $this->maybe_add_policy( $policies, $settings['wpseo_titles']['diversity_staffing_report_id'], 'diversity_staffing_report_id' ); | ||
|
||
$policies = $this->append_newest_posts( $policies ); |
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.
Considering we no longer are under time pressure to deliver this, I think this solution can be improved, eveν though it does solve the underlying issue of not showing other pages if there's one already selected for any of the options.
The issues I see with this solution:
- What I mentioned with no preparing those pages.
- The
wpseoScriptData.preferences.siteBasicsPolicies
contains a weird mixture of selected policy pages and 10 published pages. - We're adding a blocking query upon loading the Settings page (that of quering 10 public posts) and I think we would better have this behind a JS fetch.
I see with the previous solution, we already do a call to the pages
endpoint upon page load via JS, so I would explore the possibility to enhance that flow instead, in order to update each option's suggestions with the results of that call.
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.
Removed this and fixed in JS. 1 front-end request and the policies that are comming from the backend are only the ones you choose :)
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 🏗️
Some comments, from a JS perspective 😉
packages/js/src/settings/components/formik-page-select-field.js
Outdated
Show resolved
Hide resolved
packages/js/src/settings/components/formik-page-select-field.js
Outdated
Show resolved
Hide resolved
packages/js/src/settings/components/formik-page-select-field.js
Outdated
Show resolved
Hide resolved
@@ -55,7 +63,7 @@ | |||
.yst-autocomplete__options { | |||
@apply | |||
yst-absolute | |||
yst-z-10 | |||
yst-z-20 |
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 seems to be more an issue with our save footer having the same Z value of 10. But I suppose it makes sense to move our UI library to a step higher for the "popup" things. Maybe good to create an issue for the modal and select too then.
However, you should add a changelog entry for this then. Changing the style is a user-facing thing for the UI library.
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 ✅
AC ✅ |
@thijsoo I'm temporarily switching around changelog labels for UI library changelog generation. 😇 |
Release done, changelog labels switched back! 🔁 |
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:
Install
Query Monitor
plugin.Do the following steps with premium
enabled
When checking out this PR/RC refresh a front-end page and make sure there are no notices and there are no schema changes
Go to
Yoast SEO
->Settings
->Site representation
and select organisation if it is not selected.Add organisation name and logo or select a person and select a user.
Go to
Site basics
and in the third optionActionable feedback policy
open the select box and make sure there is a list of all your published pages.Select the first page and save.
Go to the frond-end and make sure there is new schema row at the bottom with key
actionableFeedbackPolicy
which holds the url to the page you selected.Fill all the select boxes and make sure all the schema pieces are there. The correct naming is:
Also make sure you can only find published pages, so no draft pages, posts or other post types.
Go to front-end of home page.
Open
Query monitor
->Database Queries
->Queries by Caller
and make sure the query withCaller
Yoast\W\S\R\Indexable_Repository->find_by_multiple_ids_and_type()
Is only called once. And it has the Id's of your pages.Empty one of the select options and save. Check that the schema is gone.
WP_Post::get_instance()
with your page ids.add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' );
Add new page...
select option and make sure you go to the new page page.User Role Editor
pluginPages
tab and remove the edit_pages option.SEO manager
role.Add Page
select option is gone from the select fields.Yoast SEO
->Settings
publishing policies
make sure all the options show up.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 and noted the work hours.Fixes #