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

i18n: Update textdomain for Event Countdown, Timeline and Blog Posts block #39554

Merged
merged 10 commits into from
Feb 26, 2020

Conversation

mkaz
Copy link
Contributor

@mkaz mkaz commented Feb 19, 2020

Changes proposed in this Pull Request

Updates were needed for Internationalization of Event Countdown, Timeline and Blog Posts blocks. This PR therefore moves the source for the Event Countdown and Timeline blocks into the FSE plugin and makes the updates to i18n, including the "full-site-editing" text-domain and add to missing strings.

The goals for FSE plugin changed so now it makes sense just to keep the source and blocks together here.

The two blocks were integrated into the build step, adding the proper package scripts. Building and testing is the same as any other FSE blocks.

Testing instructions

There is no intended functional change, for the two blocks, beyond including the source in the repo and the ability to translate as expected.

A full test to make sure nothing changed.

  1. Create a post using the Event Countdown and Timeline block prior to apply the change.
  2. Apply the change.
  3. Test you can open, edit, and view the previous blocks
  4. Test you can create, edit, and view new blocks

Tests on WordPress.com can be carried out with D39116-code.

@mkaz mkaz requested review from a team as code owners February 19, 2020 22:57
@matticbot
Copy link
Contributor

@akirk
Copy link
Member

akirk commented Feb 20, 2020

Thank you very much for this! After figuring out how to generate the dist directories (run npm run build:jetpack-timeline and npm run build:event-countdown-block) I've been able to test this with D39116-code on WordPress.com and filled in the gaps that I also committed here.

After merging this, will you create a new full-site-editing release on .org so that the changes will be available on Atomic? This will allow us to get the newly added strings translated and get them back into a language pack.

@akirk akirk changed the title Add source for Event Countdown and Timeline blocks i18n: Update textdomain for Event Countdown, Timeline and Blog Posts block Feb 20, 2020
@retrofox
Copy link
Contributor

Hi, there.
Just FYI, the blog posts block (BPB) is a block that comes from the Newspack plugin. Basically, all files in newspack-homepage-articles folder are synced from the Newspack repository through a bin script every time that it updates the Newspack plugin version.
It means that in the next updates these changes would be discarded. Here more information.

@akirk
Copy link
Member

akirk commented Feb 20, 2020

Thanks for the pointer. This means that we have a textdomain problem here, though. Are we planning to publish the newspack-blocks in the .org plugin directory?

@retrofox
Copy link
Contributor

Are we planning to publish the newspack-blocks in the .org plugin directory?

Honestly, I don't know. We could move this question to @jeffersonrabb

@simison
Copy link
Member

simison commented Feb 20, 2020

Basically, all files in newspack-homepage-articles folder are synced from the Newspack repository through a bin script every time that it updates the Newspack plugin version.
It means that in the next updates these changes would be discarded.

Here's an alternative method I'm suggesting for pulling in Newspack block: #39196 — tho not sure if it's easier or harder to plug i18n considerations into that method. It would definitely make updates easier/more fun.

@simison
Copy link
Member

simison commented Feb 20, 2020

It would be cool to switch to Sentence case from Title Case before we send these for the translation since that's the direction core is also taking.

@@ -97,6 +97,10 @@ cp -R $CODE/src/blocks/homepage-articles $TARGET/blocks/
cp -R $CODE/src/shared $TARGET/
cp -R $CODE/src/components $TARGET/

# Replace text domain
find $TARGET/blocks/ -name \*.js -exec sed -i '' "s/, 'newspack-blocks' )/, 'full-site-editing' )/g" "{}" \;
sed -i '' "s/'newspack-blocks',/'full-site-editing',/g" $TARGET/class-newspack-blocks.php
Copy link
Member

Choose a reason for hiding this comment

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

sed ing is fine and fast — did you also consider running wp-textdomain here? I'm assuming not worth for one file?

Would be great if it would handle .js files too, tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove the part of the syncing files on this process, in favor of #39196?

Copy link
Member

Choose a reason for hiding this comment

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

I talked to @simison and he said his changes are independent of that sed above.

@simison, I agree but as of now wp-textdomain only supports PHP.

@jeffersonrabb
Copy link
Contributor

Are we planning to publish the newspack-blocks in the .org plugin directory?

@akirk as of now, no plans to publish to .org.

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

mkaz and others added 8 commits February 24, 2020 12:37
- Add source code for Event Countdown block instead of just minified
- Add Event Countdown Block build script to FSE process
Add text-domain and missing strings for Title and Description
- Adds the source for Timeline block
- Fixes Internationalization text-domain and missing strings
@mkaz mkaz force-pushed the add/wpcom-blocks-src branch from f2d7a34 to 9db03e4 Compare February 24, 2020 20:38
@mkaz
Copy link
Contributor Author

mkaz commented Feb 25, 2020

Rebased and all issues addressed (and tests properly kicked).
This should be ready to go.

Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

From the FSE plugin point of view, this is looking good!

  • ✅ Added timeline and countdown blocks to a page on wpcom before syncing. Verified it looked good in editor and on front end.
  • ✅ Pulled changes locally and ran the fse plugin dev & sync command
  • ✅ Reloaded the editor on wpcom and verified that I can still make changes to those two blocks which are reflected on the front end. (and verified that I'm loading the dev version of the plugin.)

I did leave a comment D39116-code regarding the deploy step. I'm not sure if the diff was just for testing or if it was also meant to be merged there. Just want to make sure things will work well for the

P.S. once #39528 and D39062-code are merged, a link to a wpcom diff will be added to the PR if it affects FSE plugin files! It'll be nice for testing changes like this without having to do manual syncs :)

@akirk
Copy link
Member

akirk commented Feb 26, 2020

@noahtallen I was unaware of the procedure, thanks for the explanation on the Diff! I bumped the version here but then realized that this will be done separately a little later. So I guess this means that this PR can just be merged and will be shipped with the next iteration and we'll close D39116-code and get the changes online via the "normal procedure."

@noahtallen
Copy link
Contributor

So I guess this means that this PR can just be merged and will be shipped with the next iteration and we'll close D39116-code and get the changes online via the "normal procedure."

If that works for you, yes! We're currently doing updates ad-hoc. Feel free to start the process if the FG page makes sense to you :)

@noahtallen
Copy link
Contributor

Though I will say that the update process will only affect the "wp-content/plugins/full-site-editing-plugin" directory. So if those other directories in the diff need to be merged, that would also need to happen separately.

@mkaz mkaz merged commit 66545a6 into master Feb 26, 2020
@mkaz mkaz deleted the add/wpcom-blocks-src branch February 26, 2020 20:35
@mkaz mkaz mentioned this pull request Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants