Fix the RRM SetupMain story and include a VRT scenario. #9082

techanvil · 2024-07-26T17:02:19Z

Summary

Addresses issue:

Implement <PublicationSelect> component #8837

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 5.2 and PHP 7.4.
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.

github-actions · 2024-07-26T17:09:47Z

Build files for 5e0d539 have been deleted.

github-actions · 2024-07-26T17:10:05Z

Size Change: 0 B

Total Size: 1.67 MB

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	54.8 kB
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	11 kB
`./dist/assets/css/googlesitekit-authorize-application-css-********************.min.css`	846 B
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	7.35 kB
`./dist/assets/js/31-********************.js`	2.76 kB
`./dist/assets/js/32-********************.js`	2.25 kB
`./dist/assets/js/33-********************.js`	3.64 kB
`./dist/assets/js/34-********************.js`	935 B
`./dist/assets/js/35-********************.js`	892 B
`./dist/assets/js/36-********************.js`	3.12 kB
`./dist/assets/js/analytics-advanced-tracking-********************.js`	901 B
`./dist/assets/js/googlesitekit-activation-********************.js`	23.7 kB
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	59.7 kB
`./dist/assets/js/googlesitekit-adminbar-********************.js`	34.5 kB
`./dist/assets/js/googlesitekit-api-********************.js`	10 kB
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.88 kB
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	10.1 kB
`./dist/assets/js/googlesitekit-consent-mode-********************.js`	25.6 kB
`./dist/assets/js/googlesitekit-data-********************.js`	2.35 kB
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	9.03 kB
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.08 kB
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	20.4 kB
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	9.98 kB
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	25.1 kB
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	77.3 kB
`./dist/assets/js/googlesitekit-events-provider-contact-form-7-********************.js`	646 B
`./dist/assets/js/googlesitekit-events-provider-easy-digital-downloads-********************.js`	624 B
`./dist/assets/js/googlesitekit-events-provider-mailchimp-********************.js`	630 B
`./dist/assets/js/googlesitekit-events-provider-ninja-forms-********************.js`	712 B
`./dist/assets/js/googlesitekit-events-provider-optin-monster-********************.js`	675 B
`./dist/assets/js/googlesitekit-events-provider-popup-maker-********************.js`	634 B
`./dist/assets/js/googlesitekit-events-provider-woocommerce-********************.js`	657 B
`./dist/assets/js/googlesitekit-events-provider-wpforms-********************.js`	633 B
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.93 kB
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	142 kB
`./dist/assets/js/googlesitekit-modules-********************.js`	22.1 kB
`./dist/assets/js/googlesitekit-modules-ads-********************.js`	29.5 kB
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	113 kB
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	162 kB
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	22.6 kB
`./dist/assets/js/googlesitekit-modules-reader-revenue-manager-********************.js`	13.6 kB
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	58 kB
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	32 kB
`./dist/assets/js/googlesitekit-notifications-********************.js`	3.63 kB
`./dist/assets/js/googlesitekit-polyfills-********************.js`	376 B
`./dist/assets/js/googlesitekit-settings-********************.js`	74.2 kB
`./dist/assets/js/googlesitekit-splash-********************.js`	74 kB
`./dist/assets/js/googlesitekit-user-input-********************.js`	48.7 kB
`./dist/assets/js/googlesitekit-vendor-********************.js`	321 kB
`./dist/assets/js/googlesitekit-widgets-********************.js`	78.9 kB
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	61.3 kB
`./dist/assets/js/runtime-********************.js`	1.3 kB

_{compressed-size-action}

aaemnnosttv

Thanks @techanvil, just a few small things to update

aaemnnosttv · 2024-07-26T17:24:21Z

assets/js/modules/reader-revenue-manager/components/setup/SetupMain.stories.js

@@ -27,8 +32,27 @@ function Template() {

 export const Default = Template.bind( {} );
 Default.storyName = 'Default';
+Default.scenario = {
+	label: 'Modules/ReaderRevenueManager/Setup/SetupMain/Default',
+	delay: 250,


We already have a base delay in our backstop config which is much more – does this actually help or is it just copied?

Hmm - fair question. The story does have a brief loading state so I thought we might need the delay. But, maybe the default will suffice. I've removed the delay, let's see how it fares.

To be honest, I had forgotten the default is 1 second which as you say is much more and will probably be enough.

aaemnnosttv · 2024-07-26T17:26:16Z

assets/js/modules/reader-revenue-manager/components/setup/SetupMain.stories.js

@@ -27,8 +32,27 @@ function Template() {

 export const Default = Template.bind( {} );
 Default.storyName = 'Default';
+Default.scenario = {
+	label: 'Modules/ReaderRevenueManager/Setup/SetupMain/Default',


We should avoid hardcoding the label as this is constructed based on other values such as the storyName and the default title below.

Thanks - another interesting comment. Most of our stories do specify the label, but you are quite right, it shouldn't be needed.

In fact I've only found one scenario without a label in the codebase (it's one that you updated recently to remove the label :)).

site-kit-wp/assets/js/modules/ads/components/settings/SettingsView.stories.js

Line 49 in 5e0d539

Default.scenario = {};

Anyhow - another good shout, I've removed the label. Again, it would be nice to tidy the existing stories up at some point in this regard.

Yeah, I did some analysis recently and found that there are a lot of stories where the generated scenario label differed from the one explicitly referenced but yeah it would be nice to clean that up.

aaemnnosttv · 2024-07-26T17:26:27Z

assets/js/modules/reader-revenue-manager/components/setup/SetupMain.stories.js

+				.receiveGetPublications( [] );
+
+			return (
+				<WithTestRegistry registry={ registry }>


Please use WithRegistrySetup and the setup func instead – we should almost never need WithTestRegistry

Thanks for the reminder! I had forgotten this, and mostly copied the decorator from one of the existing stories which use WithTestRegistry, e.g.:

site-kit-wp/assets/js/components/Header.stories.js

Lines 286 to 300 in 7705f0c

export default {

title: 'Components/Header',

component: Header,

decorators: [

( Story ) => {

const registry = createTestRegistry();

provideSiteInfo( registry );

return (

<WithTestRegistry registry={ registry }>

<Story />

</WithTestRegistry>

);

},

],

Anyhow - good to be reminded, I've updated the story!

It would be nice to go through the existing stories at some point with a view to tidying them up in this regard...

aaemnnosttv

Great, thanks!

techanvil added 2 commits July 26, 2024 18:00

Fix the RRM SetupMain story and include a VRT scenario.

2e670a3

Add VRT reference images.

7705f0c

techanvil mentioned this pull request Jul 26, 2024

Fix the RRM SetupMain story and include a VRT scenario. #9081

Closed

18 tasks

aaemnnosttv requested changes Jul 26, 2024

View reviewed changes

techanvil added 3 commits July 26, 2024 18:44

Prefer WithRegistrySetup to WithTestRegistry.

fcf4f37

Remove scenario delay.

3b89177

Remove unneeded scenario label.

5e0d539

aaemnnosttv approved these changes Jul 26, 2024

View reviewed changes

aaemnnosttv merged commit e802a17 into main Jul 26, 2024
25 of 26 checks passed

aaemnnosttv deleted the enhancement/8837-fix-rrm-setupmain-story branch July 26, 2024 18:18

This was referenced Jul 30, 2024

Implement PublicationOnboardingStateNotice component. #9037

Merged

Tidy up and remove redundant use of label property in VRT scenarios #9103

Open

ankitrox mentioned this pull request Sep 18, 2024

Add "temporarily hidden" badge to hidden audiences in the Selection Panel #9315

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the RRM SetupMain story and include a VRT scenario. #9082

Fix the RRM SetupMain story and include a VRT scenario. #9082

techanvil commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

aaemnnosttv left a comment

aaemnnosttv Jul 26, 2024

techanvil Jul 26, 2024

techanvil Jul 26, 2024

aaemnnosttv Jul 26, 2024

techanvil Jul 26, 2024 •

edited

Loading

aaemnnosttv Jul 26, 2024

aaemnnosttv Jul 26, 2024

techanvil Jul 26, 2024 •

edited

Loading

aaemnnosttv left a comment

	export default {
	title: 'Components/Header',
	component: Header,
	decorators: [
	( Story ) => {
	const registry = createTestRegistry();
	provideSiteInfo( registry );

	return (
	<WithTestRegistry registry={ registry }>
	<Story />
	</WithTestRegistry>
	);
	},
	],

Fix the RRM SetupMain story and include a VRT scenario. #9082

Fix the RRM SetupMain story and include a VRT scenario. #9082

Conversation

techanvil commented Jul 26, 2024 • edited Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Jul 26, 2024 • edited Loading

github-actions bot commented Jul 26, 2024 • edited Loading

aaemnnosttv left a comment

Choose a reason for hiding this comment

aaemnnosttv Jul 26, 2024

Choose a reason for hiding this comment

techanvil Jul 26, 2024

Choose a reason for hiding this comment

techanvil Jul 26, 2024

Choose a reason for hiding this comment

aaemnnosttv Jul 26, 2024

Choose a reason for hiding this comment

techanvil Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

aaemnnosttv Jul 26, 2024

Choose a reason for hiding this comment

aaemnnosttv Jul 26, 2024

Choose a reason for hiding this comment

techanvil Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

techanvil commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

techanvil Jul 26, 2024 •

edited

Loading

techanvil Jul 26, 2024 •

edited

Loading