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.

Sign up for GitHub

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

Jump to bottom

Starter Page Templates: Add all SPT themes home templates to the template selector #37407

Merged
merged 5 commits into from Nov 15, 2019
Merged

Starter Page Templates: Add all SPT themes home templates to the template selector #37407

merged 5 commits into from Nov 15, 2019

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Nov 7, 2019 •
edited

Changes proposed in this Pull Request

  • Update SPT to request all SPT themes homepage templates.
  • Display these homepage templates in a new additional grouping in the SPT selector.
Before After
Screenshot 2019-11-07 at 16 46 33 Screenshot 2019-11-08 at 12 25 14

Testing instructions

  • Apply D35170-code and sandbox the API.
  • Build this PR with npx lerna run build --scope='@automattic/full-site-editing' on a site with SPT enabled (a local FSE test environment is fine, but it might lack some blocks required by the theme).
  • Create a new page on that site and make sure it shows the Starter Page Templates selector.
  • Make sure it has both the expected 9 vertical templates (Blank, Home, etc.), and a second grouping with many other templates named after their themes (e.g. Mayland, Rivington, Dalston, etc.).

Fixes #37282

@Copons Copons added [Type] Enhancement [Pri] High [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Full Site Editing labels Nov 7, 2019
@Copons Copons requested review from shaunandrews, apeatling and a team November 7, 2019 16:48
@Copons Copons requested a review from a team as a code owner November 7, 2019 16:48
@Copons Copons self-assigned this Nov 7, 2019
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=try/spt-more-homepage-templates

@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.

@shaunandrews
Copy link
Contributor

shaunandrews commented Nov 7, 2019 •
edited

I think the "Or choose a homepage..." label is way too wordy, and the spacing above it is too small.

Can we match the wording and spacing (42px) from the design:
image

Also, I noticed that the "Home" template still lives in the top group — it should be in the group with the other homepage templates.

@apeatling
Copy link
Member

+1 to @shaunandrews revisions. Let's definitely avoid the word "theme" here.

@apeatling
Copy link
Member

@shaunandrews Once we get this change deployed, I think we need more thinking on what the next revision looks like. Grouping templates by type/goal for example. Think Apple stacks.

@Copons
Copy link
Contributor Author

Copons commented Nov 8, 2019

@shaunandrews @apeatling I'm so sorry, I forgot to mention that this PR purpose is mostly to see if the backend changes make sense.

Also, I noticed that the "Home" template still lives in the top group — it should be in the group with the other homepage templates.

Ah, I totally didn't notice this!
I just assumed we wanted the current behaviour + a second group with all other home templates.
I'll update the endpoint for this.

@Copons Copons added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 8, 2019
@Copons
Copy link
Contributor Author

Copons commented Nov 8, 2019

Updated the PR for the latest backend changes, and with all the suggestions posted here.

Screenshot 2019-11-08 at 12 25 14

@Copons Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Team] Cylon [Pri] BLOCKER and removed [Status] In Progress [Pri] High labels Nov 8, 2019
obenland
obenland reviewed Nov 8, 2019
View reviewed changes
...ite-editing/full-site-editing-plugin/starter-page-templates/class-starter-page-templates.php
'_locale' => $this->get_iso_639_locale(),
'theme' => get_stylesheet(),
],
[ '_locale' => $this->get_iso_639_locale() ],
Copy link
Member

Choose a reason for hiding this comment

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

Nice

obenland
obenland approved these changes Nov 8, 2019
View reviewed changes
Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

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

Nice work @Copons

@Copons
Copy link
Contributor Author

Copons commented Nov 8, 2019

Note: I've put this on hold pending backend work.

@apeatling apeatling mentioned this pull request Nov 8, 2019
Closed
@apeatling
Copy link
Member

@Copons What backend work is needed?

@apeatling apeatling added this to In progress in Experience Issues / 0-FSE via automation Nov 8, 2019
@apeatling apeatling moved this from In progress to Ready to Merge in Experience Issues / 0-FSE Nov 8, 2019
@Copons
Copy link
Contributor Author

Copons commented Nov 9, 2019

@apeatling the diff works as it is, so worst case scenario we can just ship, but the performances are not there yet.
We have to figure out if there's a way speed up the themes list (wp_get_themes) somehow, maybe caching it. Hopefully someone else already wrote a solution for this though!

vindl
vindl approved these changes Nov 12, 2019
View reviewed changes
Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

Nice work @Copons! :shipit:

(non-blocking) Do you think it would make sense to to make the preview position fixed for larger screens too, so that we don't have to scroll back up after selecting the template at the bottom? It seems like this behavior is already in place for widths smaller than 1440px, and I'm not sure why it doesn't apply in general.

@Copons Copons force-pushed the try/spt-more-homepage-templates branch from 12b6728 to 51e32ee Compare November 14, 2019 14:04
@Copons
Copy link
Contributor Author

Copons commented Nov 14, 2019

(non-blocking) Do you think it would make sense to to make the preview position fixed for larger screens too, so that we don't have to scroll back up after selecting the template at the bottom? It seems like this behavior is already in place for widths smaller than 1440px, and I'm not sure why it doesn't apply in general.

@vindl That seems a bit complicated to fix, I guess we could tackle it in a follow-up, or actually just wait for the new design to be finalized. 🙂

@apeatling
Copy link
Member

Tested this, and I think this is fine to ship when the backend patch is ready, but please follow up on the fixed position main preview window once it's merged. Nice work @Copons!

@shaunandrews
Copy link
Contributor

In Core, we recently discussed avoiding the term "Template" in favor of "Layout." I don't consider it a blocker for this, but if we could change the term we could see how people perceive it in our next testing session.

@apeatling
Copy link
Member

@Copons I actually think we should change the wording from template to layout in this since it's already been done here: #37500

@noahtallen
Copy link
Member

Btw @shaunandrews @Copons I have this PR: #37624 which changes "Template" to "Layout" for cases outside of this PR.

@Copons Copons force-pushed the try/spt-more-homepage-templates branch from 51e32ee to ed69b6a Compare November 15, 2019 11:38
@Copons Copons merged commit 4e66cf7 into master Nov 15, 2019
Experience Issues / 0-FSE automation moved this from Ready to Merge to Done Nov 15, 2019
@Copons Copons deleted the try/spt-more-homepage-templates branch November 15, 2019 12:45
@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 Nov 15, 2019
@Copons Copons mentioned this pull request Nov 15, 2019
Closed
@apeatling
Copy link
Member

🎉👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vindl vindl vindl approved these changes

@obenland obenland obenland approved these changes

@apeatling apeatling apeatling approved these changes

@shaunandrews shaunandrews Awaiting requested review from shaunandrews

Assignees

@Copons Copons

Labels
[Goal] Full Site Editing [Pri] BLOCKER [Status] Blocked / Hold [Team] Cylon [Type] Enhancement
Projects
No open projects
Experience Issues / 0-FSE
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Editor: Add homepage templates from all template first themes to page template selector
7 participants
@Copons @matticbot @shaunandrews @apeatling @noahtallen @vindl @obenland