-
Notifications
You must be signed in to change notification settings - Fork 293
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
Implement Thank with Google tag placement infrastructure #5450
Comments
Thanks, @techanvil. Added a few comments for you. In general, I would like to ask you to use more verbose IB in the future because this one is hard to follow. I was needed to jump back and forth between IB and AC to figure out what exactly needs to be done and which section in AC IB refers to. When you write an IB think of an engineer that has never worked with this module/code before, they need to be able to understand what needs to be done by reading IB only.
The script should be registered by default and enqueued only if the current page type is allowed.
We should better use setters and getters for settings that are required only instead of passing all settings to the tag. This will make phpunit testing simpler. See how we do it for the
We need to use the |
Hi @eugene-manuilov, thanks for your review. I hope you don't mind, in response to your feedback about this being hard to follow I've rewritten the IB a bit to be more verbose and hopefully easier to follow.
Thanks for pointing this out! I've amended the IB to be specific about this point, using
Thanks for this too, I have updated the IB accordingly.
Having looked into this I don't see |
Thanks, @techanvil. IB is excellent now 🙌. IB ✔️
Ah... you are definitely right. I forgot about the content parameter 🤦... Looks good then! 👍 |
@techanvil, fyi I have updated IB a little bit to satisfy requirements for the #5451 ticket. |
@kuasha420 are you able to provide additional information on the code in the ticket description to complete TwG setup. Within the console I have pasted this code:
Since we are unable to set a publicationID within the tester plugin (all we can do is force a status) I removed the comments and made sure that the publicationID was included. Looking at the design documentation the publicationID can be any string at the moment so just added abc12345. I pasted the code within the settings and also various other places. Every time the error appears:
This is the first time we're testing the set up of TwG so need a little direction. Please could you provide instructions for us to complete the TwG setup? Thank you. |
Oh crap, Did I say console? You have to add the code in a mu-plugin or the themes 'function.php'. Apologies!!! |
@kuasha420 to be fair that was just my assumption 🤦♂️ Now you've said this, it totally helps. Thank you! |
QA Update:
|
QA Update: ✅Verified:
|
@kuasha420 @eugene-manuilov Minor nit-pick here in terms of approval: #5565 (review) This is not a bug or anything critical, but something to clean up. If you could address it in a quick follow-up PR against |
Good catch, @felixarntz. I have created a follow up PR #5616, do you mind reviewing it? |
The TwG module should receive its PHP classes to place the relevant tags. Since it is currently impossible to complete the TwG module setup as the UI is not yet available, a code snippet should be used to allow testing of the tag placement (relevant e.g. for QA).
Code snippet to fake TwG module to have its setup completed:
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Web_Tag
class for the Thank with Google should be implemented, extendingModule_Web_Tag
.register
method:wp_enqueue_scripts
action in which the main script is registered. See corresponding section below the main ACs for details on that script. The script should only be actually enqueued if the current template is for a singular entity of one of the post types from the TwGbuttonPostTypes
option.the_content
filter some time after Gutenberg blocks have been parsed in which the inline TwG button placeholder is conditionally added to the content only if the current template is for a singular entity of one of the post types from the TwGbuttonPostTypes
option and if the TwGbuttonPlacement
option starts withstatic_
. See corresponding section below the main ACs for details on that placeholder.static_auto
orstatic_below-content
: Append the placeholder to the contentstatic_above-content
: Prepend the placeholder to the contentstatic_below-first-paragraph
: Parse content to detect end of firstp
tag and inject the placeholder right after.render
method could either be used to render the placeholder or left empty if preferred from an engineering perspective.colorTheme
,buttonPlacement
, andbuttonPostTypes
settings, the class will need to provide a way to get the values of those (e.g. setter methods).register_tag
method should be added to theThank_With_Google
class and hooked intotemplate_redirect
. In this method, the newWeb_Tag
instance should be conditionally registered only if not AMP (if AMP, silently bail early as TwG does not support AMP), if the tag is not blocked, and based on the following tag guards:Tag_Verify_Guard
Tag_Environment_Type_Guard
TwG JS snippet
google_thankjs
.https://news.google.com/thank/js/v1/thank.js
TwG button placeholder
Implementation Brief
In file
includes/Modules/Thank_With_Google/Web_Tag.php
:Web_Tag
class that extendsModule_Web_Tag
.Web_Tag
classes for examples, e.g.includes/Modules/Analytics/Web_Tag.php
.buttonPlacement
andbuttonPostTypes
, as these values are explictly referenced in the AC. An instance variable and setter forcolorTheme
can be added as and when it's needed.register
method. Within this method:wp_enqueue_scripts
action hook.wp_enqueue_scripts
should be theenqueue_gtag_script
method.the_content
filter hook with a value above 9 for the priority, with the methodupdate_the_content
as the handler.do_blocks
function is added as a filter hook with priority 9, so anythe_content
hook with a higher priority will be executed after the blocks are parsed. It's worth adding a comment to the code to clarify this when it comes to implementation.do_init_tag_action()
method to initialise the tag.enqueue_gtag_script
method. Within this method.wp_register_script
, with script details as listed under TwG JS snippet in the AC.buttonPostTypes
option, useis_singular( $buttonPostTypes )
. Note that$buttonPostTypes
should be anarray
of post types.buttonPostTypes
then enqueue the script usingwp_enqueue_script
specifying only the script handle and no other arguments.wp_add_inline_script
to prepend the inline script as specified under TwG JS snippet in the AC:$position
parameter ofwp_add_inline_script
with value'before'
to prepend the inlined script.GOOGLESITEKIT_VERSION
for the current plugin version.is_singular()
to check if the current entity is a singular entity.get_permalink()
to retrieve the current permalink.get_the_title()
to retrieve the current post title.update_the_content
method with a single parameter,$content
. Within this method:buttonPostTypes
(i.e.! is_singular( $buttonPostTypes )
) bail out early.buttonPlacement
TwG setting isstatic_auto
orstatic_below-content
, return$content
appended with the placeholder.buttonPlacement
isstatic_above-content
, return$content
prepended with the placeholder.buttonPlacement
isstatic_below-first-paragraph
:</p>
in the content (this should be sufficient - it should not be necessary to use an HTML parser).</p>
.render
method to return the actual placeholder content, or simply create the placeholder directly withinupdate_the_content
.In file
includes/Modules/Thank_With_Google.php
:register
method:template_redirect
action hook, with the methodregister_tag
as the handler.register_tag
method. Within this method:register_tag
for exampleincludes/Modules/Analytics.php
.$this->context->is_amp()
to determine AMP status).is_tag_blocked()
method).Modules::Analytics::register_tag
:Tag_Verify_Guard
andTag_Environment_Type_Guard
tag guards.can_register
method, and if so:register
method.Test Coverage
Thank_With_Google
PHPUnit integration tests to check the scripts are enqueued & inlined.QA Brief
Setup
function.php
.QA
Thank with Google snippet added by Site Kit
html comments.'buttonPlacement' => 'static_auto',
: The snippet will be after the post content.'buttonPlacement' => 'static_below-content',
: The snippet will be after the post content.'buttonPlacement' => 'static_above-content',
: The snippet will be before the post content.'buttonPlacement' => 'static_below-first-paragraph',
: The snippet will be after first paragraph of the content.buttonPlacement
just change it in the mu-plugin in the first PHP snippet after setup.Changelog entry
The text was updated successfully, but these errors were encountered: