Enhancement/5451 twg widget #5577

eugene-manuilov · 2022-07-20T14:06:42Z

Summary

Addresses issue:

Implement Thank with Google supporter wall widget #5451

Relevant technical choices

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

…wg-widget.

…tly for the supporter widget preview.

…wg-widget.

techanvil

Hi @eugene-manuilov, code-wise this is looking good with just a couple of minor comments from me.

However when it comes to testing it, I'm unable to follow the QAB as specified. The reason being, although the QAB mentions simply activating the TwG module, in fact the module needs to be connected in order for the widget to be registered:

site-kit-wp/includes/Modules/Thank_With_Google.php

Lines 56 to 58 in def2970

    
           if ( ! $this->is_connected() ) { 
        
           	return; 
        
           }

At the moment it's not possibly to get as far as connecting the TwG module, unless I am missing something?

So at the moment when I add a Legacy Widget it doesn't give me the option for the TwG: Supporter Wall widget. Whereas if I comment out that check for is_connected(), it does show up. Not sure if I have missed something here, or if the QAB needs a rethink, or possibly this should be tested once TwG can be fully connected...

includes/Modules/Thank_With_Google/Supporter_Wall_Widget.php

techanvil · 2022-07-26T12:34:17Z

includes/Modules/Thank_With_Google/Supporter_Wall_Widget.php

+	public function __construct() {
+		parent::__construct(
+			self::WIDGET_ID,
+			sprintf( 'Thank with Google: %s', __( 'Supporter Wall', 'google-site-kit' ) )


The "Thank with Google" string here also needs to be translated...

I am not sure about this... As far as I know product names are not translated since they are "international" names. For example, take a look at https://www.thinkwithgoogle.com/ - you can switch between different languages and see that "Think with Google" remains untranslated.

From what I can see we are translating the service names, for example -

site-kit-wp/includes/Modules/Thank_With_Google.php

Line 117 in cc86db8

'name' => _x( 'Thank with Google', 'Service name', 'google-site-kit' ),

This can be also seen for other modules and in JS too...

Fair point, I'll update 👍

Updated, should be good now.

…wg-widget.

eugene-manuilov · 2022-07-26T16:15:51Z

However when it comes to testing it, I'm unable to follow the QAB as specified. The reason being, although the QAB mentions simply activating the TwG module, in fact the module needs to be connected in order for the widget to be registered:

At the moment it's not possibly to get as far as connecting the TwG module, unless I am missing something?

So at the moment when I add a Legacy Widget it doesn't give me the option for the TwG: Supporter Wall widget. Whereas if I comment out that check for is_connected(), it does show up. Not sure if I have missed something here, or if the QAB needs a rethink, or possibly this should be tested once TwG can be fully connected...

I have added one step to QAB to use a snippet from another ticket to fake TwG setup. Does it help?

techanvil · 2022-07-27T11:21:44Z

However when it comes to testing it, I'm unable to follow the QAB as specified. The reason being, although the QAB mentions simply activating the TwG module, in fact the module needs to be connected in order for the widget to be registered:
At the moment it's not possibly to get as far as connecting the TwG module, unless I am missing something?
So at the moment when I add a Legacy Widget it doesn't give me the option for the TwG: Supporter Wall widget. Whereas if I comment out that check for is_connected(), it does show up. Not sure if I have missed something here, or if the QAB needs a rethink, or possibly this should be tested once TwG can be fully connected...

I have added one step to QAB to use a snippet from another ticket to fake TwG setup. Does it help?

Thanks @eugene-manuilov, that did help and I've been able to connect TwG. One thing to note, though is that as well as inserting the PHP snippet I also had to run the JS snippet from the issue's QA Brief, so it might be worth a mention of this. E.g.

-Add a snippet from the https://github.com/google/site-kit-wp/issues/5450 to fake TwG setup into the functions.php file or into a mu-plugin.
+Follow the Setup instructions in the QA Brief for https://github.com/google/site-kit-wp/issues/5450 to connect the TwG module.

…wg-widget.

eugene-manuilov · 2022-07-27T13:15:13Z

However when it comes to testing it, I'm unable to follow the QAB as specified. The reason being, although the QAB mentions simply activating the TwG module, in fact the module needs to be connected in order for the widget to be registered:
At the moment it's not possibly to get as far as connecting the TwG module, unless I am missing something?
So at the moment when I add a Legacy Widget it doesn't give me the option for the TwG: Supporter Wall widget. Whereas if I comment out that check for is_connected(), it does show up. Not sure if I have missed something here, or if the QAB needs a rethink, or possibly this should be tested once TwG can be fully connected...

I have added one step to QAB to use a snippet from another ticket to fake TwG setup. Does it help?

Thanks @eugene-manuilov, that did help and I've been able to connect TwG. One thing to note, though is that as well as inserting the PHP snippet I also had to run the JS snippet from the issue's QA Brief, so it might be worth a mention of this. E.g.
-Add a snippet from the https://github.com/google/site-kit-wp/issues/5450 to fake TwG setup into the functions.php file or into a mu-plugin.
+Follow the Setup instructions in the QA Brief for https://github.com/google/site-kit-wp/issues/5450 to connect the TwG module.

Yep, sounds good to me. Updated. Thanks.

techanvil

LGTM! Nice one @eugene-manuilov

eugene-manuilov added 8 commits July 19, 2022 16:00

Add the initial supporter wall widget.

8e8ee9f

Register the widget.

71d02ca

Add hooks to load web tag for preview.

3f8378d

Merge remote-tracking branch 'origin/develop' into enhancement/5451-t…

6dbf99a

…wg-widget.

Merge remote-tracking branch 'origin/develop' into enhancement/5451-t…

a821c7c

…wg-widget.

Update hooks to register the web tag for the widget preview.

2b07973

Update the Thank with Google module to load the web tag script correc…

24e9b1b

…tly for the supporter widget preview.

Merge remote-tracking branch 'origin/develop' into enhancement/5451-t…

50bb609

…wg-widget.

eugene-manuilov marked this pull request as ready for review July 21, 2022 18:31

eugene-manuilov added 2 commits July 21, 2022 22:58

Add a description to the widget.

c83e9d7

Merge remote-tracking branch 'origin/develop' into enhancement/5451-t…

bc85b6b

…wg-widget.

techanvil requested changes Jul 26, 2022

View reviewed changes

eugene-manuilov added 2 commits July 26, 2022 18:58

Merge remote-tracking branch 'origin/develop' into enhancement/5451-t…

1ac6989

…wg-widget.

Update code style in the supporter wall widget.

48cdfc0

eugene-manuilov added 2 commits July 27, 2022 16:13

Merge remote-tracking branch 'origin/develop' into enhancement/5451-t…

78b6bd2

…wg-widget.

Update the translation for the supporter wall widget.

83a1dca

techanvil approved these changes Jul 27, 2022

View reviewed changes

techanvil merged commit 8720bad into develop Jul 27, 2022

techanvil deleted the enhancement/5451-twg-widget branch July 27, 2022 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/5451 twg widget #5577

Enhancement/5451 twg widget #5577

eugene-manuilov commented Jul 20, 2022 •

edited by techanvil

Loading

techanvil left a comment

techanvil Jul 26, 2022

eugene-manuilov Jul 26, 2022 •

edited

Loading

techanvil Jul 27, 2022

eugene-manuilov Jul 27, 2022

eugene-manuilov Jul 27, 2022

eugene-manuilov commented Jul 26, 2022

techanvil commented Jul 27, 2022

eugene-manuilov commented Jul 27, 2022

techanvil left a comment

Enhancement/5451 twg widget #5577

Enhancement/5451 twg widget #5577

Conversation

eugene-manuilov commented Jul 20, 2022 • edited by techanvil Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

techanvil left a comment

Choose a reason for hiding this comment

techanvil Jul 26, 2022

Choose a reason for hiding this comment

eugene-manuilov Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

techanvil Jul 27, 2022

Choose a reason for hiding this comment

eugene-manuilov Jul 27, 2022

Choose a reason for hiding this comment

eugene-manuilov Jul 27, 2022

Choose a reason for hiding this comment

eugene-manuilov commented Jul 26, 2022

techanvil commented Jul 27, 2022

eugene-manuilov commented Jul 27, 2022

techanvil left a comment

Choose a reason for hiding this comment

eugene-manuilov commented Jul 20, 2022 •

edited by techanvil

Loading

eugene-manuilov Jul 26, 2022 •

edited

Loading