402 implement site kit setup flow #21978

vraja-pro · 2025-01-20T10:13:20Z

Context

Summary

This PR can be summarized in the following changelog entry:

[@yoast/ui-library 0.1.0] Adds Stepper Element.
Adds unreleased Google Site kit installation flow in the dashboard.

Relevant technical choices:

This PR doesn't include the quick tour after connecting, the connecting modal and the remove permanently functionality.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Go to Yoast SEO -> General page.
Check you don't see the widget for Google Site Kit setup flow(GSK)
Enable the feature flag by adding this code ( use Snippets plugin or add to wp-config.php ):

define( 'YOAST_SEO_GOOGLE_SITE_KIT_FEATURE', true );

Go to Yoast SEO -> General page.
Check you now see the widget for GSK set up flow:
Check the flow in the widget is identical with the design flow [here]
(https://www.figma.com/proto/DXKVj4tji7M65FYCYwBFUl/Dashboard?node-id=2984-5586&t=rdGyKj954yajisjl-0&scaling=min-zoom&content-scaling=fixed&page-id=2984%3A5579&starting-point-node-id=2984%3A5586)
Check the links will automatically install and activate the plugin.
To test completion of set up, complete installation and activation and remove the exclamation sign from line 46 in packages/js/src/dashboard/components/google-site-kit-connection-widget.js
To test completion of connection, apply this patch connected-to-gsk.patch
Check you can dismiss the widget until next visit by clicking on the three dots.
The "Dismiss permanently" will not work ( for a different PR ).
The quick tour will not work ( for a different PR ).
The learn more link would not work, but will be present with params.

UI library - Stepper element

Check the stepper works as the flow above.
Check the active step label is in primary color.
Check completed step label has darker hue.
Check steps are in the right direction on rtl.

Relevant test scenarios

Changes should be tested with the browser console open
Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
Changes should be tested on different browsers
Changes should be tested on multisite

Test instructions for QA when the code is in the RC

QA should use the same steps as above.

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

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

I have tested this code to the best of my abilities.
During testing, I had activated all plugins that Yoast SEO provides integrations for.
I have added unit tests to verify the code works as intended.
If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
I have written this PR in accordance with my team's definition of done.
I have checked that the base branch is correctly set.

Innovation

No innovation project is applicable for this PR.
This PR falls under an innovation project. I have attached the innovation label.
I have added my hours to the WBSO document.

Fixes Implement Site Kit setup flow without the consent modal

add stepper to ui library export stepper fix stories args move margin out of the plugin fix story space

add translation to the stepper

coveralls · 2025-01-20T10:24:13Z

Pull Request Test Coverage Report for Build f007acaf20c302eb1fdd48ebcbcc1eb5ef88a8fb

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

30 of 54 (55.56%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall first build on 402-implement-site-kit-setup-flow at 42.142%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/js/src/dashboard/components/dashboard.js	0	4	0.0%
src/dashboard/application/configuration/dashboard-configuration.php	0	20	0.0%

Totals
Change from base Build f7f95dc739fe7266db4edb615d5011d551edaa57:	42.1%
Covered Lines:	20636
Relevant Lines:	45727

💛 - Coveralls

igorschoester

CR 🏗️

About the changelogs/label: You put enhancement while I don't think the unreleased should go in there, right? So probably you meant to signal enhancement for the UI library one. You can do both, see documentation.

AT ❓

Getting the following React warning (first run, but already on just not connected):

Warning: Failed prop type: The prop isComplete is marked as required in Stepper, but its value is undefined.

The current step svg does not center in the stepper. In the first it is off to the left/bottom? And the rest is off to the right/bottom?
No animations? The FTC' vertical stepper has one -- using our ProgressBar that would be fixed
Something seems off with the check size, maybe just the outline thing
The UI library storybook story is not responding to the inputs
The dropdown menu does not show any hover/selection effect
The dropdown menu selected looks weird:

igorschoester · 2025-01-22T07:28:11Z

packages/js/src/dashboard/components/dashboard.js

+	const googleSiteKitConfiguration = get( window, "wpseoScriptData.dashboard.google_site_kit", {
+		installed: false,
+		featureActive: false,
+		active: false,
+		setup: false,
+		connected: false,
+		installUrl: "",
+		activateUrl: "",
+		setupUrl: "",
+	} );


There was a whole point to try to make this dashboard with the eyes of it being standalone/platform agnostic.

Perhaps this widget is special in that it would never be part of standalone due to it being for a WP plugin.
However, that would mean you should invent some way of adding to the Dashboard from outside instead? I get that that is a bit strange to do when we have no clue how that would look like.

But now the dashboard has hidden expectations on the window. I think I'd prefer at least a clear API, even if that is going to change for standalone 🤔 (e.g. use props?)

igorschoester · 2025-01-22T07:29:58Z

src/dashboard/application/configuration/dashboard-configuration.php

@@ -106,6 +128,38 @@ public function get_configuration(): array {
 			)->to_array(),
 			'endpoints'               => $this->endpoints_repository->get_all_endpoints()->to_array(),
 			'nonce'                   => $this->nonce_repository->get_rest_nonce(),
+			'google_site_kit'         => $this->get_google_site_kit_configuration(),


We use camelcase for JS here for the rest.

Suggested change

'google_site_kit' => $this->get_google_site_kit_configuration(),

'googleSiteKit' => $this->get_google_site_kit_configuration(),

igorschoester · 2025-01-22T07:31:02Z

src/dashboard/application/configuration/dashboard-configuration.php

+	 *
+	 * @return array<string|bool>
+	 */
+	public function get_google_site_kit_configuration(): array {


It feels to me this should really be an 🧅 thing elsewhere. Similar to our editor script data?
And then it can be reused for the integration page.
(sorry to only mention this now)

igorschoester · 2025-01-22T07:41:05Z

packages/js/src/dashboard/components/dashboard.js

+	const [ showGoogleSiteKit, , , , setRemoveGoogleSiteKit ] = useToggleState( true );
+
+	const handleRemovePermanently = useCallback( ()=>{
+		/* eslint-disable-next-line */
+		// TODO: Implement the remove permanently functionality.
+		setRemoveGoogleSiteKit();
+	}, [ setRemoveGoogleSiteKit ] );


I feel like this too should get the initial data. Which might be part of the remove permanently scope?
However, it seems strange that you went and implemented this already while it is out of scope.

Due to the local state "Until next visit" is back already when you switch tab/page in the dashboard (e..g. to general and back). Is this what is wanted/expected?

packages/js/src/dashboard/components/google-site-kit-connection-widget.js

igorschoester · 2025-01-23T11:12:01Z

packages/ui-library/src/elements/stepper/stories.js

The whole interaction is not controllable from the inputs. It is possible to connect the inputs to the wrapper component. You can take a look at some other stories using updateArgs, like the Select one?

igorschoester · 2025-01-23T11:22:43Z

packages/js/src/dashboard/components/google-site-kit-connection-widget.js

+	const linkParams = useSelect( select => select( "@yoast/general" ).selectLinkParams(), [] );
+	const learnMorelink = addQueryArgs( "https://yoa.st/google-site-kit-learn-more", linkParams );


This makes the widget be connected to our WordPress integration already

igorschoester · 2025-01-23T11:23:59Z

packages/js/src/dashboard/components/google-site-kit-connection-widget.js

+	connected,
+	active,
+	setup,
+	installed,


We usually indicate boolean names with isX to indicate it is a boolean true/false thing.

igorschoester · 2025-01-23T11:39:23Z

packages/js/src/dashboard/components/google-site-kit-connection-widget.js

+	const { buttonProps, currentStep, isComplete } = getButtonAndStepperProps(
+		active, installed, setup, connected, installUrl, activateUrl, setupUrl );
+	return <Paper className="yst-@container yst-grow yst-max-w-screen-sm yst-p-8 yst-shadow-md yst-relative">
+		<button


This could be the tertiary variant (but then it would be color primary), or maybe it is time to add another variant to the UI library button?
I think this is not good for our focus styles, spacing etc:

In fact, the whole popup/menu thing is something the other widgets will also need.
We could chose to extract something at that point, but might be good to think about it a bit at least?

igorschoester · 2025-01-23T11:42:09Z

packages/ui-library/src/elements/stepper/index.js

+	} );
+	const stepRef = useRef( [] );
+
+	useEffect( () => {


Shouldn't this calculation happen before rendering instead of after?
E.g. there can be a point where the rendered thing is wrong for a moment in time, potentially looking funky.

vraja-pro added 3 commits January 18, 2025 00:23

stepper for ui-library

35e9827

add stepper to ui library export stepper fix stories args move margin out of the plugin fix story space

implement the stepper and design

32e9938

add translation to the stepper

implement stepper and gsk status

c8daeaa

vraja-pro added the changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog label Jan 20, 2025

vraja-pro requested a review from a team as a code owner January 20, 2025 10:13

vraja-pro added 10 commits January 20, 2025 12:45

reduce complexity and add link params

bce65d9

fix php cs

0af104a

add the remove widget ui

7e9e035

fix stepper labels colors

442008b

fix link and current step

e9150b2

fix padding in gsk widget

ace9438

dd todo comments for permanently dismissing the widget

8a0c64e

reduce eslint threshold and fix js lint

2f3bf02

add tests and fix accessibility

a952648

update svg

d29b748

igorschoester requested changes Jan 23, 2025

View reviewed changes

vraja-pro added 3 commits January 24, 2025 13:46

fixed the links, buttons and isComplete

aacdf7e

rename props and fix isComplete variable

f82bd8c

fix is complete

cc9cfa8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

402 implement site kit setup flow #21978

402 implement site kit setup flow #21978

vraja-pro commented Jan 20, 2025 •

edited

Loading

coveralls commented Jan 20, 2025 •

edited

Loading

igorschoester left a comment

igorschoester Jan 22, 2025

igorschoester Jan 22, 2025

igorschoester Jan 22, 2025

igorschoester Jan 22, 2025

igorschoester Jan 23, 2025

igorschoester Jan 23, 2025

igorschoester Jan 23, 2025

igorschoester Jan 23, 2025

igorschoester Jan 23, 2025

	'google_site_kit' => $this->get_google_site_kit_configuration(),
	'googleSiteKit' => $this->get_google_site_kit_configuration(),

		const linkParams = useSelect( select => select( "@yoast/general" ).selectLinkParams(), [] );
		const learnMorelink = addQueryArgs( "https://yoa.st/google-site-kit-learn-more", linkParams );

402 implement site kit setup flow #21978

Are you sure you want to change the base?

402 implement site kit setup flow #21978

Conversation

vraja-pro commented Jan 20, 2025 • edited Loading

Context

Summary

Relevant technical choices:

Test instructions

Test instructions for the acceptance test before the PR gets merged

UI library - Stepper element

Relevant test scenarios

Test instructions for QA when the code is in the RC

Impact check

UI changes

Other environments

Documentation

Quality assurance

Innovation

coveralls commented Jan 20, 2025 • edited Loading

Pull Request Test Coverage Report for Build f007acaf20c302eb1fdd48ebcbcc1eb5ef88a8fb

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

igorschoester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vraja-pro commented Jan 20, 2025 •

edited

Loading

coveralls commented Jan 20, 2025 •

edited

Loading