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

Fix Coutoire in the template selector preview #37685

Closed
apeatling opened this issue Nov 18, 2019 · 21 comments
Closed

Fix Coutoire in the template selector preview #37685

apeatling opened this issue Nov 18, 2019 · 21 comments
Assignees
Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites. [Type] Bug

Comments

@apeatling
Copy link
Member

apeatling commented Nov 18, 2019

The theme does not render correctly, this looks like a responsive css issue.

Screen Shot 2019-11-18 at 11 59 13 AM

@apeatling apeatling added [Type] Bug [Pri] High Address as soon as possible after BLOCKER issues [Feature Group] Appearance & Themes Features related to the appearance of sites. labels Nov 18, 2019
@apeatling apeatling added this to the Experience Issues milestone Nov 18, 2019
@apeatling
Copy link
Member Author

To be clear, this is using the Coutoire homepage layout with Maywood active as the theme.

@apeatling
Copy link
Member Author

@danieldudzic Assigning you to this one, now that the FSE changes have merged to dotcom, you should be able to reproduce this more easily.

@iamtakashi
Copy link
Contributor

It doesn't seem to be the theme-specific issue though. The inherited paddings in one of the CSS in SPT is breaking the layout. What's the purpose of this?

Screenshot 2019-11-19 at 22 13 13

@danieldudzic
Copy link
Contributor

This indeed seems to be caused by that extra padding.

@iamtakashi
Copy link
Contributor

iamtakashi commented Nov 19, 2019

I've seen some visual inconsistency between the preview and what I get in the editor with the same theme. I opened a new issue here #37742 with the test process that I took. I think the preview should be as close as possible to what the user gets once they add the layout into the editor.

@apeatling
Copy link
Member Author

@Copons For the sake of the user test would you be willing to exclude this theme from the response? Let's fix this after the test (unless you know of an easy padding fix for this).

@Copons
Copy link
Contributor

Copons commented Nov 20, 2019

@apeatling I'm totally willing (also suggested it right at the end of paYE8P-eB-p2 🙂).
I'll also exclude the Stratford layout that has a broken Cover block.
EDIT: diff at D35708-code

@iamtakashi I noticed those inconsistencies too but didn't have enough spare cycles to fully investigate them.
I'll continue the discussions in #37742 and assign it to Cylon.

@iamtakashi
Copy link
Contributor

@Copons

I'll also exclude the Stratford layout that has a broken Cover block.

I recovered the block and the layout is fine on the demo site now. What needs to be done to feed the layout picker the latest? Would Cylon be able to update it? :) If this can be automated or can be done through a MC page it would be ace so that designers don't need to bother every time blocks on page layouts break.

noticed those inconsistencies too but didn't have enough spare cycles to fully investigate them.
I'll continue the discussions in #37742 and assign it to Cylon.

👍

@Copons
Copy link
Contributor

Copons commented Nov 21, 2019

@iamtakashi I'll reenable Stratford ASAP, thanks for the fix!

I'm not sure how to automatize the process though, but adding/removing layout is a very simple API change, that doesn't involve any FSE plugin work, so it can be shipped immediately (users will still have a 1-day cache for the API response though).

@Copons
Copy link
Contributor

Copons commented Nov 21, 2019

@iamtakashi I've checked the demo site and the block looks still broken to me, nor do I see any revisions in the last month. 🤔

Screenshot 2019-11-21 at 14 22 23

(btw this only happens in the block editor, the front end looks perfectly fine as it renders the Cover block straight from its code)

@iamtakashi
Copy link
Contributor

I've checked the demo site and the block looks still broken to me, nor do I see any revisions in the last month.

@Copons I've updated it again after hitting "attempt block recovery", but if I open the page in the editor again, the block it's broken again :/ I'll see what I can do to recover the block.

@iamtakashi
Copy link
Contributor

@Copons OK, it looks like "attempt block recovery" didn't change anything other than removing the error message. Anyway, the block doesn't seem to like setting the vertical position to 0%. I've now changed it to 1% and save it. It seems working for me, but let me know if it's still broken for you.

@iamtakashi
Copy link
Contributor

@Copons

(users will still have a 1-day cache for the API response though)

Oh, does this mean that the changes that designers make on layouts will automatically be reflected in the layout picker after the cache cleared?

@Copons
Copy link
Contributor

Copons commented Nov 22, 2019

@iamtakashi Thanks this seems to have worked! ✨

About the caching: we have two caches going on with the layouts.
One in the API that stores the layouts to avoid slow too many file system searches, and one in the Starter Page Templates plugin that stores the API response.
Both are 1-day long, but aren't in sync.
The plugin one is also automatically cleared at every plugin version changes, and also it's skipped when in development environment (when the WP_DEBUG constant is enabled, as it is for example in the Gutenberg Docker setup that we use when working on FSE and SPT).

This means that changes on the demo sites will automatically appear in the layout selector in less than 1 day.

We've been keeping them at 1 day because we don't expect many urgent changes on the layouts, but let me know if y'all think we need to lower it.
(I'm personally not good at cache and performance, but I can pass your request along to more skilled people 😄).

@Copons
Copy link
Contributor

Copons commented Nov 22, 2019

Actually @iamtakashi, your fix is correct but I've just noticed that the API fetches the layouts from the headstart JSONs (in the sandbox, not in the theme repo), not straight from the demo site. 🤦‍♂

I've tried applying your fix directly to the files and it seems to work fine.
I'm currently enquiring about how to do it automatically, but I think you shouldn't have to do anything else. 👍

@iamtakashi
Copy link
Contributor

iamtakashi commented Nov 22, 2019

the API fetches the layouts from the headstart JSONs, not straight from the demo site

That was what I suspected :)

I'm currently enquiring about how to do it automatically, but I think you shouldn't have to do anything else.

Thank you!! Obviously it depends on the cost to make it happen, but it'd be nice if designers don't need to feel that they'll bother engineers whenever layouts need to be updated :)

@Copons
Copy link
Contributor

Copons commented Nov 22, 2019

@iamtakashi I'll update you as soon as I find out the procedure, which is still unknown to me and apparently all the people who worked on it are in other timezones. 😄

@WunderBart WunderBart self-assigned this Dec 16, 2019
@jeryj jeryj self-assigned this Dec 16, 2019
@jeryj
Copy link
Contributor

jeryj commented Dec 17, 2019

Using Coutoire with FSE appears to be fixed by #38065

Screenshot taken using the Maywood theme with latest from wp-calypso master and Gutenberg 7.1.0:
Screen Shot 2019-12-17 at 3 16 49 PM

@jeryj
Copy link
Contributor

jeryj commented Dec 20, 2019

After further review, it seems like this is only fixed when the FSE plugin is active, which @obenland says is not the case for everyone on WPcom yet.

@lancewillett
Copy link
Contributor

I tested this today with the "Add New" page flow in both Calypso and WP Admin. Testing with Firefox 73 on macOS Catalina with testerlance user.

I didn't see Coutoire in the Homepage layouts list any more. With that, closing as likely not a valid issue any more. (If this is a mistake, please feel free to re-open.)

Screen Shot 2020-03-09 at 10 41 13

Screen Shot 2020-03-09 at 10 41 16

@lancewillett lancewillett removed the [Pri] High Address as soon as possible after BLOCKER issues label Mar 9, 2020
@jeryj
Copy link
Contributor

jeryj commented Mar 10, 2020

I believe Coutoire was removed from the API response as an option for the Home Page layouts because of the broken layout. The template selector preview just had a big update released (#39628), so I wouldn't be surprised if this layout is fixed or at least significantly better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites. [Type] Bug
Projects
None yet
Development

No branches or pull requests

7 participants