glendaviesnz commented 5 years ago

There is no code to review for this issue as the header template has been updated in the fsemodernbusiness site.

Steps to test

Set up a new FSE site either locally or via the Horizon FSE CfT steps
Check that default modern business logo is displaying in the header instead of the image placeholder

before

after

gwwar commented 5 years ago

@apeatling the site logo placeholder is pretty prominent. Should we maybe populate the design with a logo to start with to avoid these screens? cc @iamtakashi @shaunandrews

apeatling commented 5 years ago

Yes we should, this has come up a few times in feedback. I think we should actually update this issue to reflect that.

@iamtakashi Should we use the placeholder logo from the modern business proofs?

apeatling commented 5 years ago

I probably should have opened a new issue instead, but this will work for now.

iamtakashi commented 5 years ago

I agree with starting with an actual logo image rather than a placeholder. But, would FSE themes header always start with having a logo? Or does it depend on demo site?

apeatling commented 5 years ago

It will depend on the header template being used, so it could be different depending on what the theme needs.

iamtakashi commented 5 years ago

Ok, and we'll define the header template in the demo site, correct?

apeatling commented 5 years ago

I think we can do that, but it's not really in scope for this discussion, we just need to decide what the best default logo is in the context of Modern Business.

iamtakashi commented 5 years ago

Sure, we can use the logo in the demo site instead of the placeholder.

glendaviesnz commented 5 years ago

@apeatling Can you please point me to the proofs so I can get a copy of the logo mentioned?

apeatling commented 5 years ago

@iamtakashi The logo in the demo site looks fuzzy on a retina display, do we have a 2x version of it? Is this going to be different in Maywood?

@glendaviesnz You can start with the logo at the top of https://modernbusinessdemo.wordpress.com

gwwar commented 5 years ago

What's left on this one and is this blocking for 10% testing?

glendaviesnz commented 5 years ago

As far as I can tell there are no code changes required for this. We just need to update the header template content and change the image block from

<!-- wp:image {"className":"fse-site-logo"} -->
<figure class="wp-block-image fse-site-logo"><img alt=""/></figure>
<!-- /wp:image -->

to

<!-- wp:image {"className":"fse-site-logo"} -->
<figure class="wp-block-image size-large fse-site-logo"><img src="https://modernbusinessdemo.files.wordpress.com/2019/06/logo-modern-3.png" alt=""/></figure>
<!-- /wp:image -->

Just need to hunt down that code for snapshooting the template data ...

glendaviesnz commented 5 years ago

@qwwar I can't access the templates any more at fsemodernbusiness/wp-admin/edit.php?post_type=wp_template - gives a console error - did we move these to the main modernbusiness demo site already?

For future reference details about how to snapshot the new header with default image at D31195-code

glendaviesnz commented 5 years ago

So for some reason the patch D31361-code had broken access to the fse demo site for me, I think maybe because I had classic editor set for my user from past edits of the templates. I can get in and edit them my temporarily reverting that on my sandbox - @gwwar - do you want me to go ahead and add that logo to the header template on fsemodernbusiness?

I have added and issue for the problem of forcing gutenberg for users that have chosen classic - https://github.com/Automattic/wp-calypso/issues/35804

gwwar commented 5 years ago

do you want me to go ahead and add that logo to the header template on fsemodernbusiness?

Sure let's do that for now, though we'll also need to make sure we reapply changes to Maywood when we switch over.

glendaviesnz commented 5 years ago

This is now done and the default modern business logo now displays instead of the placeholder if you go through the FSE setup flow

noahtallen commented 5 years ago

Just went through the CfT flow again, and this is definitely working as expected!

For follow up, I guess we just need to make sure the templates for maywood/etc also do this.

I guess I'll close this since there's no merging to do :)

gwwar commented 5 years ago

@glendaviesnz can we update the header block property on the image to add an explicit align center?

<!-- wp:image {"align":"center","className":"size-large fse-site-logo"} -->
<div class="wp-block-image size-large fse-site-logo"><figure class="aligncenter"><img src="https://modernbusinessdemo.files.wordpress.com/2019/06/logo-modern-3.png" alt=""/></figure></div>
<!-- /wp:image -->

I'm seeing the following currently while testing https://github.com/Automattic/themes/pull/1341

It also looks like we're hotlinking. Not sure if we need to tidy before the 10% test, but we should for 100%. @obenland any ideas for what we'd need to do for side-loading? At worst, this looks small enough where even a base64 encoded image would be preferable.

apeatling commented 5 years ago

I've re-pinged Takashi on this one, it needs to be a 2x version of this image.

iamtakashi commented 5 years ago

@iamtakashi The logo in the demo site looks fuzzy on a retina display, do we have a 2x version of it? Is this going to be different in Maywood?

Sorry for the delay in response. We could use a different logo for Maywood. It's not a big deal if that's not convenient though. Here is a big version of a new logo. Let me know if there is any issue.

maywood-logo

glendaviesnz commented 5 years ago

It also looks like we're hotlinking. Not sure if we need to tidy before the 10% test, but we should for 100%. @obenland any ideas for what we'd need to do for side-loading?

There is code at /full-site-editing/full-site-editing-plugin/starter-page-templates/class-wp-rest-sideload-image-controller.php for side-loading images, but would be a bit of work to get this working in tandem with the fse template creation api - I would suggest we defer for the 100%

At worst, this looks small enough where even a base64 encoded image would be preferable.

Have tried base64 encoded data: version of the new logo but on saving the base64 data is stripped - I am guessing the the KSES filters remove this.

So, any thoughts on whether we just make do with a hotlinked image for the 10% trial, or is there another way to get base64 encoded version accepted?

apeatling commented 5 years ago

Hot linking seems fine for the test, since we are still hot linking other images in templates right now (until we figure out side loading).

apeatling commented 5 years ago

@glendaviesnz Let's update this to use the high res logo Takashi provided.

obenland commented 5 years ago

Hot linking seems fine for the test, since we are still hot linking other images in templates right now

I'd strongly advise against hotlinking more images. We'll never be able to change or remove those images once we start doing that, even for a small amount of sites. It's part of the reason why I've held back updating those template images with optimized versions so far.

obenland commented 5 years ago

35588 added the utility functions to communicate with the endpoint, #34839 has the code that integrates the side-loading endpoint in the templates selector. We've had a a fair bit of performance troubles with it, especially for multiple images though.

apeatling commented 5 years ago

Thanks @obenland, that's a good point. I'm concerned about the performance issues of side loading as a blocker for the test.

obenland commented 5 years ago

If it's just this one image with a size of 35kb it should be fine. We've not had many problems with the blog template so far, Professional with its 12+ images is where it becomes pretty bad. I do realize though that implementing that sideloading approach might delay the test.

gwwar commented 5 years ago

I'll create a new issue for the hotlinking fix, since that sounds like a decent amount of work.

glendaviesnz commented 5 years ago

@gwwar have added the explicit centre alignment now.

gwwar commented 5 years ago

Thank @glendaviesnz verified that this populates with the attribute on a new site:

Closing this out and will add a second one for the hotlinking

apeatling commented 5 years ago

@gwwar @glendaviesnz Based on the above, it doesn't look like the new logo Takashi linked? The hotlink URL needs to be updated to:

https://maywooddemo.files.wordpress.com/2019/08/maywood-logo.png

With the appropriate size set.

Automattic / wp-calypso

FSE: Add a default logo placeholder image #35456

Steps to test

35588 added the utility functions to communicate with the endpoint, #34839 has the code that integrates the side-loading endpoint in the templates selector. We've had a a fair bit of performance troubles with it, especially for multiple images though.