extract shuffle into js-utils and use it to randomize the themes on /start flow #57344

roo2 · 2021-10-26T02:28:42Z

Changes proposed in this Pull Request

Fixes #56832 by giving a random sort to themes presented in the design picker. This will prevent certain themes from being prioritized and will allow us to see which themes are the most often selected.

There was an existing random function used in the design-picker package, but we could not share this since we use the theme API to load designs instead of the design pickers getAvailableDesigns method. Since we're migrating away from lodash (p4TIVU-9Bf-p2) and shuffle is a lodash function, I've migrated shuffleArray into our js-utils library.

Testing instructions

Test the design picker with
/start/design-setup-site?siteSlug=YOUR_SITE.wordpress.com Test the existing /newflow by going to/new` and navigating to the design picker step.

matticbot · 2021-10-26T02:34:19Z

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-17810
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-17810&env=jetpack

…start flow

matticbot · 2021-10-26T05:37:40Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~33 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding       +112 B  (+0.0%)      +33 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~26 bytes added 📈 [gzipped])

name    parsed_size           gzip_size
signup       +125 B  (+0.0%)      +26 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~58 bytes added 📈 [gzipped])

name                                   parsed_size           gzip_size
async-load-signup-steps-design-picker        +97 B  (+0.1%)      +58 B  (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

tyxla

Thanks for considering not using Lodash! I think shuffle is a good candidate for @automattic/js-utils now that we need to use it in multiple packages.

I can see this PR is still a draft, but I thought some early feedback could be helpful.

In a nutshell, @automattic/js-utils@0.1.0 was never released, so we should keep the version intact.

tyxla · 2021-10-26T09:00:35Z

packages/js-utils/package.json

@@ -1,6 +1,6 @@
 {
 	"name": "@automattic/js-utils",
-	"version": "0.1.0",
+	"version": "0.2.0",


0.1.0 has never been released, so there's no need to bump the version here. Let's make sure to revert all the related changes to package.json and yarn.lock.

good point!

tyxla · 2021-10-26T09:00:51Z

packages/js-utils/src/index.ts

@@ -1 +1,2 @@
 export { default as uniqueBy } from './unique-by';
+export { default as shuffle } from './shuffle';


Nit: Should we keep the list alphabetically sorted?

Maybe we should have a look at https://www.npmjs.com/package/eslint-plugin-sort-exports

roo2 · 2021-10-26T09:33:17Z

thanks for the feedback @tyxla ! I've made those changes, hopefully the only thing left to do is get CI passing 🤞

tyxla · 2021-10-26T10:11:06Z

packages/design-picker/package.json

@@ -32,6 +32,7 @@
 	},
 	"dependencies": {
 		"@automattic/calypso-config": "^1.0.0-alpha.0",
+		"@automattic/js-utils": "^0.1.0",


Let's also add js-utils to the references list in tsconfig.json of the design-picker package.

That should resolve the Cannot find module '@automattic/js-utils' or its corresponding type declarations. error.

tyxla · 2021-10-26T10:11:37Z

packages/tsconfig.json

@@ -6,6 +6,7 @@

 	// Reference all TS packages
 	"references": [
+		{ "path": "./js-utils" }, // define js-utils first so that it will be build before packages that depend on it


Small typo here:

Suggested change

{ "path": "./js-utils" }, // define js-utils first so that it will be build before packages that depend on it

{ "path": "./js-utils" }, // define js-utils first so that it will be built before packages that depend on it

I removed this, I think that this order was only relevant because I'd forgotten to add it to the tsconfig of design-picker as a dependency 😂

roo2 · 2021-10-26T10:36:11Z

Thanks for finding that I had forgot to add the dependency to design-picker/tsconfig.json. I was able to remove that non-alphabetical hack and the unit tests are passing now :)

tyxla

Works well and looks good from my perspective 👍 🚢

arthur791004

Well done!

…start flow (#57344)

roo2 requested review from p-jackson and a team October 26, 2021 02:28

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 26, 2021

roo2 removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 26, 2021

roo2 marked this pull request as draft October 26, 2021 02:30

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 26, 2021

roo2 force-pushed the update/randomize-site-designs branch from 51364e1 to c5fd682 Compare October 26, 2021 03:53

extract shuffle into js-utils and use it to randomize the themes on /…

bd08002

…start flow

roo2 force-pushed the update/randomize-site-designs branch from 401e974 to bd08002 Compare October 26, 2021 04:39

change build order to avoid error on clean install

907add8

tyxla reviewed Oct 26, 2021

View reviewed changes

address feedback

b9bb60e

tyxla reviewed Oct 26, 2021

View reviewed changes

add dependency

1be8c12

roo2 marked this pull request as ready for review October 26, 2021 10:27

tyxla approved these changes Oct 26, 2021

View reviewed changes

arthur791004 approved these changes Oct 27, 2021

View reviewed changes

only shuffle the feature themes, not blank canvas

30bc064

roo2 merged commit e8b48b9 into trunk Nov 1, 2021

roo2 deleted the update/randomize-site-designs branch November 1, 2021 00:22

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 1, 2021

nelsonec87 pushed a commit that referenced this pull request Dec 9, 2021

extract shuffle into js-utils and use it to randomize the themes on /…

78b5106

…start flow (#57344)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extract shuffle into js-utils and use it to randomize the themes on /start flow #57344

extract shuffle into js-utils and use it to randomize the themes on /start flow #57344

roo2 commented Oct 26, 2021

matticbot commented Oct 26, 2021 •

edited

Loading

matticbot commented Oct 26, 2021 •

edited

Loading

tyxla left a comment

tyxla Oct 26, 2021

roo2 Oct 26, 2021

tyxla Oct 26, 2021

scinos Oct 27, 2021

roo2 commented Oct 26, 2021

tyxla Oct 26, 2021

roo2 Oct 26, 2021

tyxla Oct 26, 2021

roo2 Oct 26, 2021

roo2 commented Oct 26, 2021

tyxla left a comment

arthur791004 left a comment

		@@ -1 +1,2 @@
		export { default as uniqueBy } from './unique-by';
		export { default as shuffle } from './shuffle';

	{ "path": "./js-utils" }, // define js-utils first so that it will be build before packages that depend on it
	{ "path": "./js-utils" }, // define js-utils first so that it will be built before packages that depend on it

extract shuffle into js-utils and use it to randomize the themes on /start flow #57344

extract shuffle into js-utils and use it to randomize the themes on /start flow #57344

Conversation

roo2 commented Oct 26, 2021

Changes proposed in this Pull Request

Testing instructions

matticbot commented Oct 26, 2021 • edited Loading

matticbot commented Oct 26, 2021 • edited Loading

tyxla 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

roo2 commented Oct 26, 2021

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

roo2 commented Oct 26, 2021

tyxla left a comment

Choose a reason for hiding this comment

arthur791004 left a comment

Choose a reason for hiding this comment

matticbot commented Oct 26, 2021 •

edited

Loading

matticbot commented Oct 26, 2021 •

edited

Loading