-
Notifications
You must be signed in to change notification settings - Fork 898
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
Add hiive html attributes to ftc #20678
Conversation
clicked_select_image has been added also to the image select box, and clicked_remove_image has been added to the remove image link.
Fixed also some indentation.
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.
A few points to discuss
packages/js/src/first-time-configuration/first-time-configuration-steps.js
Outdated
Show resolved
Hide resolved
packages/js/src/first-time-configuration/tailwind-components/configuration-stepper-buttons.js
Outdated
Show resolved
Hide resolved
packages/js/src/first-time-configuration/tailwind-components/configuration-stepper-buttons.js
Outdated
Show resolved
Hide resolved
...c/first-time-configuration/tailwind-components/steps/social-profiles/social-profiles-step.js
Show resolved
Hide resolved
Co-authored-by: Enrico Battocchi <enrico.battocchi@gmail.com>
Co-authored-by: Enrico Battocchi <enrico.battocchi@gmail.com>
Co-authored-by: Enrico Battocchi <enrico.battocchi@gmail.com>
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 🏗️
- The test instructions don't seem to mention the actions at all. You could introduce those and verify the data you expect is actually there
- While peeking at the corresponding PR there might be a case where the old vs new strict equals check results in false positives, when the order of the values changed. Not sure if that is an actual issue
- Checking the sheet' events and your code:
- all these
changed_
andset_
are not actually there, but via the actions. Should the sheet be updated, or how does that work? - as noted in code comments:
- there is no
clicked_replace_image
- typo in the
clicked_save_changes
(missing underscore)
- there is no
clicked_back
is noted asclicked_go_back
(for all the steps) in the sheet
- all these
packages/js/src/first-time-configuration/tailwind-components/configuration-stepper-buttons.js
Outdated
Show resolved
Hide resolved
@@ -79,6 +80,7 @@ export default function ImageSelect( { | |||
id={ url ? id + "__replace-image" : id + "__select-image" } | |||
className="yst-button yst-button yst-button--secondary yst-mr-2" | |||
onClick={ onSelectImageClick } | |||
data-hiive-event-name="clicked_select_image" |
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 suppose by the difference clicked_select_image
knows if it is a replace or not 🤔
But it could be a separate event too, as you could argue the same for remove.
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.
Done.
packages/js/src/first-time-configuration/tailwind-components/configuration-stepper-buttons.js
Outdated
Show resolved
Hide resolved
packages/js/src/first-time-configuration/tailwind-components/configuration-stepper-buttons.js
Outdated
Show resolved
Hide resolved
...first-time-configuration/tailwind-components/steps/personal-preferences/newsletter-signup.js
Outdated
Show resolved
Hide resolved
* @param array The old values of the options. | ||
* @param array The options that failed to be saved. | ||
*/ | ||
\do_action( 'wpseo_post_update_social_profiles', $params, $old_values, $failures ); |
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.
- You used in the add_action
wpseo_ftc_post_update_social_profiles
elsewhere! - Maybe signal this as
@internal
? Or create documentation PR?
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.
Done.
* @param bool Whether the option failed to be stored. | ||
*/ | ||
// $success is negated to be aligned with the other two actions which pass $failures. | ||
\do_action( 'wpseo_post_update_enable_tracking', $params['tracking'], $option_value, ! $success ); |
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.
- You used in the add_action
wpseo_ftc_post_update_enable_tracking
elsewhere! - Maybe signal this as
@internal
? Or create documentation PR?
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.
Done.
@@ -104,7 +123,17 @@ public function set_site_representation( $params ) { | |||
* @return object The response object. | |||
*/ | |||
public function set_social_profiles( $params ) { | |||
$failures = $this->social_profiles_helper->set_organization_social_profiles( $params ); | |||
$failures = $this->social_profiles_helper->set_organization_social_profiles( $params ); | |||
$old_values = $this->get_old_values( \array_keys( $this->social_profiles_helper->get_organization_social_profile_fields() ) ); |
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 noticing that the
get_organization_social_profile_fields
uses but does not check if the filtered return value. Maybe add a safety(array)
there to ensure this keeps working? - Also, I see this is the same as for person (with
get_person_social_profile_fields
), but you haven't changed that code in your PR (set_person_social_profiles
method). Why?
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 first point has been addressed.
About the second point, set_person_social_profiles
is dead code, I'm creating a tech debt issue to address that too. 🙂
* | ||
* @return array The old values. | ||
*/ | ||
private function get_old_values( $fields_names ) : array { |
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.
If you're adding the return type hinting, why not also the argument?
(then I noticed the filters and missing type checks 😅 -- still, might want to make those safer instead)
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.
Done.
packages/js/src/first-time-configuration/tailwind-components/configuration-stepper-buttons.js
Outdated
Show resolved
Hide resolved
…onfiguration-stepper-buttons.js Co-authored-by: Igor <35524806+igorschoester@users.noreply.github.com>
…onfiguration-stepper-buttons.js Co-authored-by: Igor <35524806+igorschoester@users.noreply.github.com>
…onfiguration-stepper-buttons.js Co-authored-by: Igor <35524806+igorschoester@users.noreply.github.com>
…onfiguration-stepper-buttons.js Co-authored-by: Igor <35524806+igorschoester@users.noreply.github.com>
…teps/personal-preferences/newsletter-signup.js Co-authored-by: Igor <35524806+igorschoester@users.noreply.github.com>
It is unnecessary because the 'description' field is not used anymore, so it's never gonna be there.
The conditional about the "description" field has been removed because the field itself has been previously removed.
That shouldn't be a problem because, in case of a false negative, the new and old value are passed to the
They are there 😁
Should be fixed.
Should be fixed.
Should be fixed. |
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 ✅
ACC ✅ |
Context
wp-module-data
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
FirstTimeConfigurationSteps
and all the buttons inpackages/js/src/first-time-configuration/tailwind-components/configuration-stepper-buttons.js
have been refactored to standardise the way html ids are computed.src/actions/configuration/first-time-configuration-action.php
a methodget_old_values
has been introduced and called inset_site_representation
andset_social_profiles
actions to allow old options values to be passed to the events tracking code.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Preliminary steps
Yoast Test Helper
to reset the First time configuration stateTools
->Yoast Test
Reset First time configuration progress
in theYoast SEO
sectionVerify the presence of the intended html attributes in the First-time configuration flow
Yoast SEO
->General
->First-time configuration
tab, and check for each step the presence of the following attribute/value pair:SEO Data configuration step:
data-hiive-event-name="clicked_start_data_optimization"
in theStart SEO data optimization
buttondata-hiive-event-name="clicked_continue | data optimization"
in theContinue
buttonSite representation step:
data-hiive-event-name="clicked_select_image"
in theSelect image
button and in the image select boxReplace image
and the attribute/value pair will becomedata-hiive-event-name="clicked_replace_image"
data-hiive-event-name="clicked_remove_image"
in theRemove image
linkdata-hiive-event-name="clicked_continue | site representation"
in theSave and continue
buttondata-hiive-event-name="clicked_go_back | site representation"
in theGo back
buttonSocial profiles step (for organisations):
data-hiive-event-name="clicked_add_profile"
in theAdd another profile
buttondata-hiive-event-name="clicked_continue | social profiles"
in theSave and continue
buttondata-hiive-event-name="clicked_go_back | social profiles"
in theGo back
buttonSocial profiles step (for people):
data-hiive-event-name="clicked_update_or_add_profile | social profiles"
in theupdate or add social profiles to this user profile
linkdata-hiive-event-name="clicked_continue | social profiles"
in theSave and continue
buttondata-hiive-event-name="clicked_go_back | social profiles"
in theGo back
buttonPersonal preferences step:
data-hiive-event-name="clicked_signup | personal preferences"
in theSign up!
buttondata-hiive-event-name="clicked_continue | personal preferences"
in theSave and continue
buttondata-hiive-event-name="clicked_go_back | personal preferences"
in theGo back
buttonFinish configuration step:
data-hiive-event-name="clicked_to_onboarding_page"
in theLearn how to increase your rankings with Yoast SEO
buttondata-hiive-event-name="clicked_seo_dashboard"
in theOr go to your SEO dashboard
linkVerify the presence of the intended html attributes after the completion of the First-time configuration
Edit
buttons which appear aside each step's title have the following attribute/value pair attached:data-hiive-event-name="clicked_edit | data optimization"
data-hiive-event-name="clicked_edit | site representation"
data-hiive-event-name="clicked_edit | social profiles"
data-hiive-event-name="clicked_edit | personal preferences"
Save changes
button in theSite representation
step has the attribute/value pairdata-hiive-event-name="clicked_save changes | site representation"
Save changes
button in theSocial profiles
step has the attribute/value pairdata-hiive-event-name="clicked_save changes | social profiles"
Save changes
button in thePersonal preferences
step has the attribute/value pairdata-hiive-event-name="clicked_save changes | personal preferences
Test the hooks get and output the correct data
wp-config.php
file/* That's all, stop editing! Happy publishing. */
debug.log
file inwp-content/
remove itdebug.log is
, typetail -f debug.log
: this command will output anything that will be written to the \debug.log` file, so you can see in real the events being logged herewp-content/mu-plugins
folder, create two new files namedlistener.php
andyoast-listener.php
listener.php
the following snippetyoast-listener.php
the following snippetYoast test helper
: this will ensure you start with an empty first time configurationYoast SEO
->General
and start the first time configurationSite representation
step, set only the Website name and clickSave and continue
tail
command, you should see the following line:Event: set_website_name, Data: {"category":"ftc_site_representation"}
Go back
to return to theSite representation
stepOrganization name
, choose a logo, updateWebsite name
amd clickSave and continue
Go back
to return to theSite representation
stepSave and continue
Event: changed_site_representation, Data: {"category":"ftc_site_representation","data":{"label_key":"site_representation","new_value":"person"}}
Go back
to return to theSite representation
stepname
dropdown and clickSave and continue
Event: set_name, Data: {"category":"ftc_site_representation"}
Go back
to return to theSite representation
stepname
dropdown and clickSave and continue
Event: changed_name, Data: {"category":"ftc_site_representation","data":{"label_key":"name","new_value":YOUR_NEW_VALUE}}
Go back
to return to theSite representation
stepSave and continue
Event: changed_site_representation, Data: {"category":"ftc_site_representation","data":{"label_key":"site_representation","new_value":"company"}}
Social profiles
step, fillfacebook
,twitter
with a twitter user name (i.e. withouthttps://twitter.com
), add another profile and clickSave and continue
Go back
to return to theSocial profiles
stepfacebook
valu, changetwitter
value by pre-pending to the usernamehttps://www.twitter.com
and clickSave and continue
changed_facebook_profile, Data: {"category":"ftc_personal_profiles","data":{"label_key":"facebook_profile","new_value":"https:\/\/www.facebook.com\/YOUR_NEW_VALUE"}}
Personal preferences
step, change the tracking option toYes, you can track my site data
and clickSave and continue
Event: set_usage_tracking, Data: {"category":"ftc_tracking"}
*Click the
Edit
button beside thePersonal preferences
step, change back the tracking option and clickSave changes
Event: changed_usage_tracking, Data: {"category":"ftc_tracking","data":{"label_key":"usage_tracking","new_value":false}}
Relevant test scenarios
Test instructions for QA when the code is in the RC
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 https://github.com/Yoast/reserved-tasks/issues/114