move page template modal into its own package #49661

roo2 · 2021-02-03T14:20:15Z

Changes proposed in this Pull Request

Moves page layout selector into its own package as the first step of the page layout selector redesign.
In order to enable future reuse and reduce the size of the ETK plugin
I used the same version numbers of dependencies that had been used in the ETK ( most of them use the * latest version)

Testing instructions

Build the code and sync to your sandbox with

cd apps/editing-toolkit
yarn
yarn dev --sync

#49240

matticbot · 2021-02-03T14:20:17Z

Test live: https://calypso.live/?branch=update/move-layout-picker-to-package

matticbot · 2021-02-03T14:28:35Z

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D56412-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

matticbot · 2021-02-03T14:34:42Z

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

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

name                 parsed_size           gzip_size
entry-gutenboarding       +169 B  (+0.0%)     +356 B  (+0.1%)

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

Sections (~1 bytes removed 📉 [gzipped])

name             parsed_size           gzip_size
woocommerce            +64 B  (+0.0%)       -1 B  (-0.0%)
settings               +64 B  (+0.0%)       -1 B  (-0.0%)
reader                 +64 B  (+0.0%)       -1 B  (-0.0%)
plugins                +64 B  (+0.0%)       -1 B  (-0.0%)
jetpack-connect        +64 B  (+0.0%)       -1 B  (-0.0%)
hosting                +64 B  (+0.0%)       -1 B  (-0.0%)
account                +64 B  (+0.0%)       -1 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 (~679 bytes added 📈 [gzipped])

name                                                 parsed_size           gzip_size
async-load-design-wordpress-components-gallery           +1339 B  (+0.2%)     +679 B  (+0.4%)
async-load-quick-language-switcher                         +64 B  (+0.0%)       -1 B  (-0.0%)
async-load-design-playground                               +64 B  (+0.0%)       -1 B  (-0.0%)
async-load-design-blocks                                   +64 B  (+0.0%)       -1 B  (-0.0%)
async-load-design                                          +64 B  (+0.0%)       -1 B  (-0.0%)
async-load-calypso-components-web-preview-component        +64 B  (+0.0%)       -1 B  (-0.0%)
async-load-calypso-blocks-nav-unification-modal            +64 B  (+0.1%)       -1 B  (-0.0%)
async-load-calypso-blocks-editor-checkout-modal            +64 B  (+0.0%)       -1 B  (-0.0%)
async-load-automattic-search                               +64 B  (+0.1%)       -1 B  (-0.0%)

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.

ramonjd · 2021-02-04T04:26:13Z

I didn't have much time to give it a proper code review, but I took it for a spin. It works! First time. No JS/PHP errors I can see so far.

Will continue tomorrow.

Thanks!

ramonjd · 2021-02-08T01:53:47Z

Tested:

Page picker appears and functions as expected from:

Home screen "Add new page link"
Side bar "Add new page link"
The new page button on the pages

Translated previews work as expected.

Nice 👌

@roo2 Do you think this PR should be broken up into two PRs:

The first on adds the code to the packages directory (to be deployed first)
The second implements the new package in the ETK

I'm only suggesting this as a precautionary measure.

This PR could be eventually merged with both changes if we were sure of them, but it might be safer for testing since someone might do an ETK release before we are ready. Also for ease of rollback. What do you think?

I added @Automattic/team-calypso as reviewers in case there was anything we should be aware of when extracting packages. Context: the page picker has a use case in onboarding as well :)

roo2 · 2021-02-08T05:02:45Z

as you suggested @ramonjd I've updated this change to be in two parts, the new package is in this PR, and then, #49805 updates the ETK to use the package. This does have the risk of potentially losing changes to /apps/editing-toolkit/editing-toolkit-plugin/starter-page-templates/ made between when this is merged and when #49805 is merged. And also to test is, I think it's easiest to actually git cherrypick f545cab3225281c3c33574743e947c4df4dae203 the commit from #49805 and then yarn dev --sync

packages/page-template-modal/package.json

packages/page-template-modal/src/components/block-preview.js

packages/page-template-modal/.eslintrc.js

packages/page-template-modal/package.json

andrewserong · 2021-02-08T06:04:32Z

Neat idea! @roo2 just a heads-up I've merged in #49695 which hides the Featured category from the page layout picker in page-template-modal/index.js so that change will need to be merged in to packages/page-template-modal/src/components/page-template-modal.js before this lands 🙂

It's just updating:

if ( ! ( key in templateGroups ) ) {

To become:

// Temporarily skip the 'featured' category so that we can expose it at another time.
if ( key !== 'featured' && ! ( key in templateGroups ) ) {

packages/page-template-modal/src/components/template-selector-preview.js

p-jackson

👍 @roo2

I know you're still working on this, but I had a thought about the <PageTemplatesPlugin> thing. I actually am wondering whether that logic should be left in the ETK in some way. It details with things like "do this if this version of GB, otherwise do that", and deals with where state should saved. That seems fairly editor specific.

But if we wanted to open the layout picker in gutenboarding, or in the customiser maybe, then we probably wouldn't want to use any of that logic from <PageTemplatesPlugin>. I think we'd want to just use the <PageTemplateModal> component and pass in our own props for when to open it.

I'm not sure whether that means <PageTemplatesPlugin> needs to be split in half and some of the logic stays in the package, or whether all belongs in the ETK.

roo2 · 2021-02-09T13:26:36Z

hmm that's a really interesting point @p-jackson 🤔 I think yea you're right, we should change this to only extract the modal and not the plugin with its gutenberg specific stores etc.

Apart from that, the other feedback is addressed, @andrewserong's change is incorporated and I've updated this PR to include updating the ETK to use the package in one fell swoop again in this PR (for ease of testing and git integrity), after discussion p1612845014028000-slack-CRWCHQGUB

noahtallen

I'm curious, what do y'all think about naming this something other than "Page Template Modal"? Template has a very specific purpose in the context of FSE, and isn't the same as what we're doing here. IMO it would be nice to change its name to one of the newer variations (page patterns?) while we have the chance.

ramonjd · 2021-02-09T22:29:37Z

IMO it would be nice to change its name to one of the newer variations (page patterns?) while we have the chance.

I agree. The component might not even be used as modal.

Page pattern/layout selector/picker?

I think I need a reminder of whether "layout" is appropriate here as well 🤣 Look at PCYsg-t0i-p2 for refs

noahtallen · 2021-02-09T22:44:04Z

Totally! I like "page pattern" way better than "page layout" myself

roo2 · 2021-02-10T03:33:38Z

@noahtallen I think it's a good idea to rename but not to do it in this PR because I think it will take quite a lot of changes if we keep all of the classnames etc consistent. And @p-jackson is currently working on this part of the code so renaming now would make it harder for him to merge his changes p1612913238087800-slack-CRWCHQGUB

roo2 · 2021-02-15T03:06:09Z

packages/page-template-modal/webpack.config.js

+
+module.exports = () => {
+	return getBaseWebpackConfig( {
+		WP: true,


edit: this line is necessary because we use core @WordPress components

roo2 · 2021-02-15T03:07:38Z

@p-jackson I've updated the PR to include scss now, I also split the scss file into what's related to the modal and whats related to the button that opens it.

roo2 · 2021-02-15T03:31:38Z

@p-jackson actually, I'm just going back to work out why I can't build this without adding that custom webpack config file, I shouldn't need it but got errors bundling the scss without it. I thought there was a bug with the default config in calypso-build but I wasn't able to reproduce that so now I'm investigating this again.

Thanks for you patience!

roo2 · 2021-02-15T03:44:43Z

@p-jackson OK, I think I've worked out what was going on, I did need the custom webpack.config.js just because the component uses custom @wordpress components so WP needs to be enabled in the build 👍.

I wasn't able to reproduce the errors that I'd thought were a calypso-build bug that I saw on Friday and everything seems to be working 👍

scinos · 2021-02-15T04:47:19Z

packages/page-template-modal/package.json

@@ -0,0 +1,56 @@
+{
+	"name": "@automattic/page-template-modal",
+	"version": "1.0.0",


This is version 1.0.0, but ETK depends on version 1.0.0-alpha.0. They should be the same version, right?

scinos · 2021-02-15T04:56:15Z

packages/page-template-modal/package.json

+	},
+	"scripts": {
+		"clean": "npx rimraf dist",
+		"build": "calypso-build",


I think this should be transpile instead of calypso-build? transpile is what other packages use, it produces the dirs dist/esm and dist/cjs (declared above in this package.json) and you won't need a custom wepbkac.config.js

this was to make sure that the scss file gets built and output as css in dist.

When we use this package form the ETK, we don't actually use the built css from dist, rather, we import the scss file directly, so maybe it doesn't matter?

But yea, the motivation for using calypso-build was so that the css gets built too

little bit of background up discussion with @p-jackson p1613363841195300-slack-CRWCHQGUB

scinos

Use transpile to build the package

roo2 · 2021-02-15T05:36:13Z

thanks for the feedback & discussion @scinos I've updated to use transpile && copy-assets and updated the version nubers to match.

packages/page-template-modal/src/index.js

packages/page-template-modal/src/styles/page-template-modal.scss

p-jackson

It's looking good!

As well as the couple notes I made, I think we can also delete the entire webpack.config.js file now?

outdated

autumnfjeld · 2021-02-15T06:53:12Z

apps/editing-toolkit/editing-toolkit-plugin/starter-page-templates/index.scss

@@ -0,0 +1,31 @@
+@import '~@automattic/page-template-modal/src/styles/page-template-modal';
+
+// Sidebar modal opener goo.


goo? :) As in glue that is holding something together? That might not be clear to next dev who comes along. :)

roo2 · 2021-02-15T07:07:39Z

thanks @p-jackson I've made the changes you suggested, @autumnfjeld I just removed that comment, I don't think it added anything in its new location

scinos · 2021-02-16T05:44:16Z

This PR has introduced lots of package duplications. While that is not a problem on itself, it can indicate that the new package is using different versions than it was using before. If it wasn't expected it may suggest a problem in the new dependencies.

I fixed the dependencies in #50114

roo2 requested review from lsl, autumnfjeld, ramonjd and a team February 3, 2021 14:20

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

ramonjd assigned roo2 Feb 3, 2021

ramonjd added the Page Layouts label Feb 3, 2021

ramonjd mentioned this pull request Feb 3, 2021

New Page Layout Picker: investigation and define tech specs #49240

Closed

4 tasks

ramonjd requested a review from a team February 8, 2021 01:46

roo2 mentioned this pull request Feb 8, 2021

Use the page template modal package #49805

Closed

roo2 force-pushed the update/move-layout-picker-to-package branch from 74d1d53 to 9555b50 Compare February 8, 2021 04:23

scinos reviewed Feb 8, 2021

View reviewed changes

ramonjd reviewed Feb 9, 2021

View reviewed changes

packages/page-template-modal/src/components/template-selector-preview.js Outdated Show resolved Hide resolved

andrewserong mentioned this pull request Feb 9, 2021

[Blocked] Starter page templates: Remove iframe from preview #49101

Closed

autumnfjeld mentioned this pull request Feb 9, 2021

New Page Layout Picker: Implement the new design including design image previews #49587

Closed

10 tasks

roo2 force-pushed the update/move-layout-picker-to-package branch from a75bc25 to 0b7e65d Compare February 9, 2021 07:20

p-jackson reviewed Feb 9, 2021

View reviewed changes

noahtallen reviewed Feb 9, 2021

View reviewed changes

roo2 requested a review from p-jackson February 10, 2021 03:45

minor refactor remove unnecessary code

d5d7b8b

roo2 force-pushed the update/move-layout-picker-to-package branch from a527f2f to d5d7b8b Compare February 15, 2021 03:01

roo2 commented Feb 15, 2021

View reviewed changes

roo2 requested a review from p-jackson February 15, 2021 03:06

scinos reviewed Feb 15, 2021

View reviewed changes

scinos suggested changes Feb 15, 2021

View reviewed changes

update to use transpile, correct import order

a94a5a6

roo2 requested a review from scinos February 15, 2021 05:36

scinos approved these changes Feb 15, 2021

View reviewed changes

p-jackson reviewed Feb 15, 2021

View reviewed changes

packages/page-template-modal/src/index.js Outdated Show resolved Hide resolved

p-jackson reviewed Feb 15, 2021

View reviewed changes

packages/page-template-modal/src/styles/page-template-modal.scss Outdated Show resolved Hide resolved

p-jackson reviewed Feb 15, 2021

View reviewed changes

autumnfjeld reviewed Feb 15, 2021

View reviewed changes

roo2 added 2 commits February 15, 2021 16:56

remove outdated code

80119dd

remove redundant comment

2905733

roo2 merged commit 892a94a into trunk Feb 15, 2021

roo2 deleted the update/move-layout-picker-to-package branch February 15, 2021 21:57

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

andrewserong mentioned this pull request Feb 18, 2021

Editing Toolkit: Update to v2.20 #50204

Merged

20 tasks

ramonjd mentioned this pull request Feb 18, 2021

Page layout selector: Spacing missing between rows #48127

Closed

autumnfjeld mentioned this pull request Feb 23, 2021

New Page Layout Picker: refactor to rename to Page Patterns #50363

Closed

p-jackson mentioned this pull request Mar 11, 2021

New Page Layout Picker: rename "templates" to "page patterns" #50971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move page template modal into its own package #49661

move page template modal into its own package #49661

roo2 commented Feb 3, 2021 •

edited

matticbot commented Feb 3, 2021

matticbot commented Feb 3, 2021

matticbot commented Feb 3, 2021 •

edited

ramonjd commented Feb 4, 2021

ramonjd commented Feb 8, 2021 •

edited

roo2 commented Feb 8, 2021

andrewserong commented Feb 8, 2021

p-jackson left a comment

roo2 commented Feb 9, 2021

noahtallen left a comment

ramonjd commented Feb 9, 2021

noahtallen commented Feb 9, 2021

roo2 commented Feb 10, 2021 •

edited

roo2 Feb 15, 2021 •

edited

roo2 commented Feb 15, 2021

roo2 commented Feb 15, 2021

roo2 commented Feb 15, 2021

scinos Feb 15, 2021

scinos Feb 15, 2021

roo2 Feb 15, 2021 •

edited

roo2 Feb 15, 2021

scinos left a comment

roo2 commented Feb 15, 2021

p-jackson left a comment

autumnfjeld Feb 15, 2021

roo2 commented Feb 15, 2021

scinos commented Feb 16, 2021 •

edited

		@@ -0,0 +1,31 @@
		@import '~@automattic/page-template-modal/src/styles/page-template-modal';

		// Sidebar modal opener goo.

move page template modal into its own package #49661

move page template modal into its own package #49661

Conversation

roo2 commented Feb 3, 2021 • edited

Changes proposed in this Pull Request

Testing instructions

matticbot commented Feb 3, 2021

matticbot commented Feb 3, 2021

matticbot commented Feb 3, 2021 • edited

ramonjd commented Feb 4, 2021

ramonjd commented Feb 8, 2021 • edited

roo2 commented Feb 8, 2021

andrewserong commented Feb 8, 2021

p-jackson left a comment

Choose a reason for hiding this comment

roo2 commented Feb 9, 2021

noahtallen left a comment

Choose a reason for hiding this comment

ramonjd commented Feb 9, 2021

noahtallen commented Feb 9, 2021

roo2 commented Feb 10, 2021 • edited

roo2 Feb 15, 2021 • edited

Choose a reason for hiding this comment

roo2 commented Feb 15, 2021

roo2 commented Feb 15, 2021

roo2 commented Feb 15, 2021

scinos Feb 15, 2021

Choose a reason for hiding this comment

scinos Feb 15, 2021

Choose a reason for hiding this comment

roo2 Feb 15, 2021 • edited

Choose a reason for hiding this comment

roo2 Feb 15, 2021

Choose a reason for hiding this comment

scinos left a comment

Choose a reason for hiding this comment

roo2 commented Feb 15, 2021

p-jackson left a comment

Choose a reason for hiding this comment

autumnfjeld Feb 15, 2021

Choose a reason for hiding this comment

roo2 commented Feb 15, 2021

scinos commented Feb 16, 2021 • edited

roo2 commented Feb 3, 2021 •

edited

matticbot commented Feb 3, 2021 •

edited

ramonjd commented Feb 8, 2021 •

edited

roo2 commented Feb 10, 2021 •

edited

roo2 Feb 15, 2021 •

edited

roo2 Feb 15, 2021 •

edited

scinos commented Feb 16, 2021 •

edited