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 makepot script using wp-i18n makepot #750

Merged
merged 6 commits into from
Mar 26, 2019
Merged

Fix makepot script using wp-i18n makepot #750

merged 6 commits into from
Mar 26, 2019

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Mar 15, 2019

Address #751

#658 was a first attempt at fixing the mobile pot file generation.

Since then babel-plugin-makepot was removed from gutenberg and the official way is to use wp i18n make-pot instead. This PR updates our makepot script to use this.

Testing Instructions

  • Have xgettext installed
  • Run yarn makepot
  • Check that ./gutenberg.pot and gutenberg-mobile.pot file were generated in the root folder
  • gutenberg-mobile.pot should only contain the 4 following strings:
    • ADD BLOCK HERE
    • Move block up
    • Move block down
    • Remove content
      All other strings in ./src are already part of gutenberg.pot.

In a followup PR, we'll want to remove those 4 strings from this repository so all strings from this project are translated by the glopress gutenberg project

@Tug Tug added the [Type] Enhancement Improves a current area of the editor label Mar 15, 2019
@Tug Tug self-assigned this Mar 15, 2019
@Tug Tug requested a review from koke March 15, 2019 12:30
@koke
Copy link
Member

koke commented Mar 15, 2019

  • Move block up
  • Move block down

Note for future: I see Gutenberg has similar labels, but they say "Move up" and "Move down". Perhaps we could reuse those.

@Tug
Copy link
Contributor Author

Tug commented Mar 15, 2019

Yeah good idea, let's start the conversation here in the meantime 👍
@SylvesterWilmott What do you think of those copy changes in guteneberg-mobile?

  • Move block up => Move up
  • Move block down => Move down
  • Remove content => Remove Block

Concerning ADD BLOCK HERE I haven't found equivalent string in gutenberg. There's Add block but it doesn't convey the information that the block will be inserted in place of this message.

@SylvesterWilmott
Copy link

Actually we are using similar language in a future version of stats which allows a repositioning of blocks:

Move up
Move down
Remove from insights

In this case I would even consider simplifying:

Remove Block => Remove

@Tug
Copy link
Contributor Author

Tug commented Mar 21, 2019

Ok I updated the strings, now we only have ADD BLOCK HERE in gutenberg-mobile.

I feel we should still keep this yarn makepot script in here to be able to check the strings we have that are only in gb-mobile.

@Tug
Copy link
Contributor Author

Tug commented Mar 25, 2019

@koke not a priority but if you have some time to review this :)

"wp": "php -d memory_limit=512M bin/wp-cli.phar",
"premakepot": "yarn makepot:gutenberg",
"makepot": "bash -c \"xgettext -f <( find . -path './src/*.js' ! -path 'node_modules' -print ) --from-code=UTF-8 -k__ -k_n -k_x -k_nx -x ./gutenberg.pot -o ./gutenberg-mobile.pot\"",
"makepot:gutenberg": "yarn clean:gutenberg && yarn wp i18n make-pot ./gutenberg --ignore-domain gutenberg.pot",
Copy link
Member

Choose a reason for hiding this comment

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

Why is the clean:gutenberg needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we want to avoid having source paths in the pot file for build and build-module files. I have tried to exclude those using the make-pot command but it's not very flexible so I'm not sure it's possible to do it cleanly.

@koke
Copy link
Member

koke commented Mar 25, 2019

Have xgettext installed

Should we add some check to make sure xgettext is present and give a better error message if not? Is it worth mentioning in the README?

@Tug
Copy link
Contributor Author

Tug commented Mar 25, 2019

Should we add some check to make sure xgettext is present and give a better error message if not? Is it worth mentioning in the README?

Well in theory we could use wp i18n make-pot but it doesn't seem to support flow syntax at the moment. I think using xgettext is a hack at the moment and since the process of updating the strings in WPAndroid and WPiOS is manual I don't think it's worth spending more time on it. The goal is to actually have an empty POT file generated here and remove the need for that feature.

Let's add a better error message though 👍

@Tug Tug merged commit 8153e3f into develop Mar 26, 2019
@Tug Tug deleted the fix/makepot-2 branch March 26, 2019 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants