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

FSE Plugin: Add Blog Page Layouts and Deploy All Layouts to Mag 16 #41364

Closed
iamtakashi opened this issue Apr 22, 2020 · 33 comments
Closed

FSE Plugin: Add Blog Page Layouts and Deploy All Layouts to Mag 16 #41364

iamtakashi opened this issue Apr 22, 2020 · 33 comments

Comments

@iamtakashi
Copy link
Contributor

We've made some simple blog layouts that are ready to be added to the layout selector.

screenshot-2020-04-17-at-18 10 49

I've already added these layouts to the master site, and I've also made a static screenshot for each and uploaded to the headstart media site.

I'm looking at the FG page about Starter page templates, but I think I need dev help :)

cc @obenland @ianstewart @johnHackworth

@obenland
Copy link
Member

I think I need dev help

What is it that you need help with?

@iamtakashi
Copy link
Contributor Author

iamtakashi commented Apr 22, 2020

My first question is...

I can see this in the FG page.

You should create the screenshot to match the existing format. alt text should be authored to be no longer than two sentences and should describe the Layout as clearly as possible within the constraints.

Where should this alt text be added? Is it to the images in the media library in the Headstart media site?

@obenland
Copy link
Member

That could indeed be clearer. I updated the docs to reflect that they need to be added to the $template_meta array in Starter_Page_Templates::add_starter_page_templates(), together with the template's category.

@iamtakashi
Copy link
Contributor Author

iamtakashi commented Apr 22, 2020

@obenland, thanks for clarifying the doc!

I've made a diff for the $template_meta array in D42287-code. Can you take a look at it if it looks good?

The next step is to generate the .json annotation file, isn't it?

@obenland
Copy link
Member

I've not had a chance to test it as I'm getting ready for an FSE update but it looks good!
Yes, updating the annotation files is next

@iamtakashi
Copy link
Contributor Author

I need to head out for today, but this is a question for tomorrow.

Do I need to run the wpcli command to generate the json file while the addition to $template_meta is in my sandbox? (Sorry, if this doesn't make sense or is too obvious to you.)

@obenland
Copy link
Member

It's not obvious, I had to double-check myself. Yes, they need to be in your sandbox so that information is picked up when the annotation script runs. If you want to make extra-sure, feel free to commit that ahead of time.

@iamtakashi
Copy link
Contributor Author

iamtakashi commented Apr 23, 2020

Thanks for the hand-holding. Much appreciated!

I have updated the diff D42287-code with an updated m1 json file.

The next item on the instruction is to generate a linguine file for translations, but is there anything else I should do before moving on to it? I have manually checked the new post_contents in the json by unescaping it and paste it to new pages, but it'd be nice to be able to test it in the layout selector otherwise I feel I'm kind of doing this blindly.

If you also think it's good to test it before moving on to translation, some addition about a testing instruction on the FG page may be helpful for people like me :)

@obenland
Copy link
Member

obenland commented Apr 23, 2020

With the templates in the JSON you'll be able to test it in the API with public-api sandboxed.
In the developer console, this will return them:
image

To test them in the template selector itself you'll need to install the plugin on a local site (so the API request gets sandboxed) and have WP_DEBUG set to true (so the response doesn't get cached)

@iamtakashi
Copy link
Contributor Author

iamtakashi commented Apr 23, 2020

Thanks, I can see the added blog layouts in the developer console. Good.

However, I can't see any blog layouts as well as some other layouts in the selector at the moment. I have Block Experiments, Full Site Editing, and Jetpack(dev mode) installed and activated, but I assume I need a connected Jetpack, and some layouts that require the Jetpack connection are hidden. I think I need to set up my local install better so that all the Jetpack blocks are available.

@iamtakashi
Copy link
Contributor Author

@obenland All the blocks used in the new blog layouts are available in my local install, and I can see all the new layouts that I've added in my local install 🎉

The next item on the instruction is to generate a linguine file that can be sent to our translation service, but I'm wondering the change in my sandbox needs to be tested and deployed before moving on.

What is the most appropriate next action? Please advice.

@obenland
Copy link
Member

I'm not sure they need to be, but they can be if you prefer to ship them as soon as possible.
Having said that, there's been more of a push to release new features translated and for all mag16 languages simultaneously.

@iamtakashi
Copy link
Contributor Author

We've decided to wait for translation for mag 16 to deploy them. I'll start the translation process tomorrow.

@iamtakashi
Copy link
Contributor Author

I've submitted the annotation for translation. I'll wait for an email about how to deploy them when the translation is done.

@iamtakashi
Copy link
Contributor Author

@akirk Hello! The translation strand 59167 was reviewed and has been sent for translation. How long do you think it'd take to be translated?

@akirk
Copy link
Member

akirk commented May 5, 2020

Some translators are on holidays this week, we hope to get all languages early next week.

@iamtakashi
Copy link
Contributor Author

iamtakashi commented May 12, 2020

Received the translation while I was AFK.

As @obenland warned in the comment of this post pbFcYn-1A-p2, The contact Info blocks on the layouts are broken in other languages. The map block also seems broken.

@obenland, would it be best to fix it in the json files?

@obenland
Copy link
Member

Correct, yes, in those translations

@iamtakashi
Copy link
Contributor Author

Noting here that all the Subscription block is also broken in other languages. The same thing @obenland experienced in Block Patterns: Automattic/jetpack#15076 (comment)

@sirreal
Copy link
Member

sirreal commented May 13, 2020

I've created Automattic/jetpack#15776, which describes the underlying issue with the Subscription Form block.

From what I've understood, there may be a workaround for these layouts. You'll need to change the button text so that it is not the default.

This block (with different button text):

Screen Shot 2020-05-13 at 13 29 13

Will result in this block:

<!-- wp:jetpack/subscriptions {"submitButtonText":"Subscribe to get all the latest news from my awesome feed"} -->
<div class="wp-block-jetpack-subscriptions">[jetpack_subscription_form show_only_email_and_button="true" custom_background_button_color="undefined" custom_text_button_color="undefined" submit_button_text="Subscribe to get all the latest news from my awesome feed" submit_button_classes="undefined" show_subscribers_total="false" ]</div>
<!-- /wp:jetpack/subscriptions -->

I understand that Subscribe to get all the latest news from my awesome feed would be part of the pack sent for localization and could be properly localized. @obenland is that accurate?

Compare that to the default:

Screen Shot 2020-05-13 at 13 31 29

<!-- wp:jetpack/subscriptions -->
<div class="wp-block-jetpack-subscriptions">[jetpack_subscription_form show_only_email_and_button="true" custom_background_button_color="undefined" custom_text_button_color="undefined" submit_button_text="Subscribe" submit_button_classes="undefined" show_subscribers_total="false" ]</div>
<!-- /wp:jetpack/subscriptions -->

In this case, the use of the default combined with block attribute parsing results in the block being invalidated. Here's an example of 2 blocks, first with the default "Subscribe" then with non-default "Subscribe!"

  • Saved in English
    Screen Shot 2020-05-13 at 13 36 40
  • Opened in Spanish for editing:
    Screen Shot 2020-05-13 at 13 36 30

Changing Subscribe to Subscribe! prevents the block invalidation.

@iamtakashi Will that unblock the template work?

@iamtakashi
Copy link
Contributor Author

@sirreal Thanks for looking into this issue, and I appreciated you made an issue for it.

This is what I see in the json file for fr that is broken at the moment. And I've been trying to edit it to fix, but I'm having trouble to make it work. Could you give me steps to fix?

<!-- wp:jetpack/subscriptions -->\n<div class=\"wp-block-jetpack-subscriptions\">[jetpack_subscription_form show_only_email_and_button=\"true\" custom_background_button_color=\"undefined\" custom_text_button_color=\"undefined\" submit_button_text=\"Sign Up\" submit_button_classes=\"undefined\" show_subscribers_total=\"false\" ]</div>\n<!-- /wp:jetpack/subscriptions -->

@sirreal
Copy link
Member

sirreal commented May 15, 2020

@iamtakashi If you open the code editor and replace the subscriptions block with this (look for wp:jetpack/subscriptions) does it work?

<!-- wp:jetpack/subscriptions {"submitButtonText":"Sign up"} -->
<div class="wp-block-jetpack-subscriptions">[jetpack_subscription_form show_only_email_and_button="true" custom_background_button_color="undefined" custom_text_button_color="undefined" submit_button_text="Sign up" submit_button_classes="undefined" show_subscribers_total="false" ]</div>
<!-- /wp:jetpack/subscriptions -->

@akirk
Copy link
Member

akirk commented May 15, 2020

I don't have enough experience with it but it seems to me that for translation, it'd be best to have our blocks use source: html. By default it is stored in the HTML comment which is left alone during translation. Thus, this incompatibility arises. If the block read the HTML, it would receive the translated texts.

I realize that this means that we'd have to rewrite our blocks (and isn't helpful in this situation) but it's the best solution I know so far. Maybe we need to build something custom to try to do this when sending back the translated text but I am not sure how easy it'd be.

@iamtakashi
Copy link
Contributor Author

@sirreal Oh, I see. I now know how to change the button text in the json :) and your suggestion works beautifully! Much appreciated!!

@sirreal
Copy link
Member

sirreal commented May 15, 2020

No problem 👍 Note that it's repeated and changed in both places. It will likely need to match to prevent invalidation: {"submitButtonText":"Sign up"} and submit_button_text="Sign up".

(Translators should be aware of that as well.)

@iamtakashi
Copy link
Contributor Author

I've fixed all the translations and updated D42287-code.

@obenland Would you mind testing the diff? I can see one of the unit tests has failed, but I'm not sure what that means or how to fix it :)

@iamtakashi
Copy link
Contributor Author

BTW, some homepage layouts are still broken, but since the sources are different, those should be fixed separately.

@iamtakashi
Copy link
Contributor Author

@akirk Do you imagine we have to do this manual fixing for all the block instances on all the layouts every time we add a new layout and update the json file? Or do we need to fix new instances on the new layout? We're planning to keep adding layout, and I obviously want to avoid this situation again. Would the Contact Info, Map, and Subscribe blocks need to be fixed before we add a new layout?

@akirk
Copy link
Member

akirk commented May 19, 2020

I think we have two options:

  1. Ensure that our blocks only use the data from HTML. When translating the HTML, the HTML comments are left as they are since in the Gutenberg case its structure is not well-defined.
  2. Add custom translation deploy code that tries to rewrite the Gutenberg HTML comment based on the translations, like you are doing manually.

Honestly, I don't have experience with source: "html" yet, and how much work it is to convert a block from comment JSON to that.

So maybe we'd first go for option 2; we'll need to consider each block with custom code, I think, since the JSON/HTML mapping might be custom (here I saw a snake_case vs camelCase which we can handle, I think).

For the path forward, I hope we can encourage everyone to move to source: "html"?

@iamtakashi
Copy link
Contributor Author

Thank you, @akirk. Am I right to understand that we'll have the same situation next time we want to deploy a new layout unless we take one of the options you suggested?

Just to share, this was what I needed to do to fix the map block instances in the layouts for other languages.

Original markup of a Map block that was broken in non-English languages:

<!-- wp:jetpack/map {\"points\":[],\"zoom\":12,\"mapCenter\":{\"lng\":151.207006,\"lat\":-33.868288}} -->\n<div class=\"wp-block-jetpack-map\" data-map-style=\"default\" data-map-details=\"true\" data-points=\"[]\" data-zoom=\"12\" data-map-center=\"{\"lng\":151.207006,\"lat\":-33.868288}\" data-marker-color=\"red\" data-show-fullscreen-button=\"true\"></div>\n<!-- /wp:jetpack/map -->

Fixed by replacing it with:

<!-- wp:jetpack/map {\"points\":[],\"zoom\":12,\"mapCenter\":{\"lng\":151.207006,\"lat\":-33.868288}} -->\n<div class=\"wp-block-jetpack-map\" data-map-style=\"default\" data-map-details=\"true\" data-points=\"[]\" data-zoom=\"12\" data-map-center=\"{&quot;lng&quot;:151.207006,&quot;lat&quot;:-33.868288}\" data-marker-color=\"red\" data-show-fullscreen-button=\"true\"></div>\n<!-- /wp:jetpack/map -->

@iamtakashi iamtakashi changed the title FSE Plugin: Add Blog Page Layouts FSE Plugin: Add Blog Page Layouts and Deploy All Layouts to Mag 16 May 19, 2020
@iamtakashi
Copy link
Contributor Author

Am I right to understand that we'll have the same situation next time we want to deploy a new layout unless we take one of the options you suggested?

@akirk Sorry, I don't think I've asked my question with the right words. What I'm worried about is that when we update the json for a new layout and send it to the translators, what I changed manually this time would be wiped out, and I need to do the same again.

@obenland
Copy link
Member

I can see one of the unit tests has failed, but I'm not sure what that means or how to fix it :)

I re-ran the tests and they're green now. You can do that by clicking on the build that failed and the click "Restart Build" in the top right corner.

I'm a bit strapped for time right now, maybe @ianstewart can help test these changes?

@iamtakashi
Copy link
Contributor Author

D42287-code has been deployed after a review by @p-jackson (Thank you!)

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

No branches or pull requests

5 participants