Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Mar 11, 2021

Changes proposed in this Pull Request

  • Refactor JS so the "templates" concept is instead called "patterns" or "page patterns" in the code
  • Leave PHP and API as is. We will need to change this more carefully so that it can be deployed without fatals (given the deployment process for ETK has changed recently)
  • Update e2e page objects with new CSS selectors

Initially proposed here (#49661 (review)) when we refactored the modal into it's own package. The word "template" has become very overloaded. The things chosen by the new page modal are really more like "patterns" (i.e. the pattern inserter in core Gutenberg), except they're for a whole page. Hence "Page Patterns".

I also considered calling the package @automattic/new-page-modal, since it's the modal that appears when creating a new page. But that's just what we happen to use it for now. Since it's a package it could be used anywhere one needed to select a page pattern.

FYI @noahtallen

Testing instructions

  • e2e tests pass (selectors have changed)
  • apply diff to sandbox and smoke test the feature still works
  • verify that no changes have been made to the API (i.e. the starterPageTemplatesConfig global) and that it won't cause fatals when deploying

Related to #50363

@p-jackson p-jackson self-assigned this Mar 11, 2021
@p-jackson p-jackson requested a review from a team as a code owner March 11, 2021 02:04
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 11, 2021
@matticbot
Copy link
Contributor

@p-jackson p-jackson changed the base branch from trunk to types/add-types-to-page-layout-modal March 11, 2021 02:05
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@noahtallen
Copy link
Contributor

Cool! Taking a quick look at the build log for ETK, I see the changed files look pretty safe:

Screen Shot 2021-03-10 at 7 29 08 PM

(Link to the lines in the build output)

It looks like from the perspective of the build artifact, only one JS file has changed, and that wouldn't be an asset file anyways, so it should be safe to go

@p-jackson p-jackson force-pushed the types/add-types-to-page-layout-modal branch from 1142f0d to 6af8bb9 Compare March 15, 2021 04:39
Copy link
Contributor

@roo2 roo2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this and it all LGTM 👍
I think it would also make sense to update the naming in the starter-page-templates subproject of the ETK in this PR, we still use templates in there.
But that's up to you

🚢 once the modal itself is deployed and rebased ✅

@p-jackson
Copy link
Member Author

Thanks @roo2 👍

I think it would also make sense to update the naming in the starter-page-templates subproject of the ETK in this PR, we still use templates in there.

Definitely want to rename this, but we're just going to have to have a think how to do this with the new deployment process. We'll need to have one PR that adds a copy of the files with a new name, and then another PR that removes the old files. And perhaps temporarily the JS will need to be able to deal with either starterPageTemplatesConfig.templates or starterPageTemplatesConfig.pagePatterns. This is because during deployment there could be a window where the old JS is being served by the new PHP, or the new JS has been served up by the old PHP.

@p-jackson p-jackson force-pushed the types/add-types-to-page-layout-modal branch from 995dd93 to 3188a55 Compare March 15, 2021 21:19
@p-jackson p-jackson force-pushed the update/rename-page-pattern-modal branch from facd793 to 767c14a Compare March 15, 2021 22:59
@p-jackson p-jackson force-pushed the types/add-types-to-page-layout-modal branch 5 times, most recently from 377b80c to 41ee893 Compare March 18, 2021 21:35
@p-jackson p-jackson force-pushed the update/rename-page-pattern-modal branch from 767c14a to 5392c8b Compare March 18, 2021 22:11
@p-jackson
Copy link
Member Author

I believe e2e tests are failing because the I've changed some test selectors to use the new classname. However because this is a change to the ETK build, the calypso e2e tests are still being run against the production version of ETK. This should be fixed once this PR merges and the new ETK build is released.

@p-jackson p-jackson force-pushed the types/add-types-to-page-layout-modal branch from 41ee893 to cbc4c26 Compare March 19, 2021 03:35
Base automatically changed from types/add-types-to-page-layout-modal to trunk March 23, 2021 21:11
@p-jackson p-jackson force-pushed the update/rename-page-pattern-modal branch from 5392c8b to 0f3d163 Compare March 24, 2021 02:58
@p-jackson p-jackson merged commit ad4901b into trunk Mar 24, 2021
@p-jackson p-jackson deleted the update/rename-page-pattern-modal branch March 24, 2021 03:37
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 24, 2021
import React, { useCallback } from '@wordpress/element';

const INSERTING_HOOK_NAME = 'isInsertingPageTemplate';
const INSERTING_HOOK_NAMESPACE = 'automattic/full-site-editing/inserting-template';
const INSERTING_HOOK_NAME = 'isInsertingPagePattern';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋🏼 Wanted to let you know that this rename broke the from_template_selector property in our tracking events. PR, for more context: #53640

I humbly recommend a more thorough code search in the future to catch any consumers of the existing hook 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that! I did search for usage but clearly I wasn't thorough enough (I don't know how I missed it given the usage in wpcom-block-editor is in the same repo 🤦‍♂️). Losing 3 months worth of that prop is frustrating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants