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

Extensions: drop i18n wrapper #11743

Merged
merged 3 commits into from
Apr 2, 2019
Merged

Extensions: drop i18n wrapper #11743

merged 3 commits into from
Apr 2, 2019

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Mar 28, 2019

Closes: #11656

Changes proposed in this Pull Request:

  • Drop the now-obsolete i18n helper in extensions/utils
  • Change __, _n, _x, _nx() imports to import directly from @wordpress/i18n
  • Add jetpack textdomain as last argument to all those functions

Testing instructions:

  • Verify that blocks still work
  • (Not sure if we currently expect translations to work? They might live in a different place than they will once this is merged and used for string extraction.)

Proposed changelog entry for your changes:

Blocks: Drop i18n wrapper, use @wordpress/i18n directly.

@sirreal sirreal added [Focus] i18n Internationalization / i18n, adaptation to different languages [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack in progress labels Mar 28, 2019
@sirreal sirreal added this to the 7.3 milestone Mar 28, 2019
@sirreal sirreal self-assigned this Mar 28, 2019
@simison
Copy link
Member

simison commented Mar 28, 2019

Do we now need to add jetpack textdomain in translation functions?

@sirreal sirreal changed the title IN PROGRESS: drop i18n wrapper Extensions: drop i18n wrapper Mar 28, 2019
@sirreal sirreal force-pushed the removew/i18n-wrapper branch from ab043d7 to 780b1d1 Compare March 28, 2019 16:38
@sirreal
Copy link
Member Author

sirreal commented Mar 28, 2019

Do we now need to add jetpack textdomain in translation functions?

Yes, I believe so.

@ockham
Copy link
Contributor

ockham commented Mar 29, 2019

This could do the trick

function addTextdomain( j, root, name ) {
  	return root
		.find( j.CallExpression, { callee: { type: 'Identifier', name } } )
		.forEach( path => {
			j( path ).replaceWith( () => {
				path.node.arguments = path.node.arguments.concat( j.stringLiteral( 'jetpack' ) );
              	return path.node;
			} );
		} )
		.toSource( { quote: 'single' } );
}

export default function transformer( file, api ) {
	const j = api.jscodeshift;
	var ret = file.source;
	ret = addTextdomain( j, j( ret ), '__' ); 
 	ret = addTextdomain( j, j( ret ), '_n' );
   	ret = addTextdomain( j, j( ret ), '_x' );
   	ret = addTextdomain( j, j( ret ), '_nx' );

	return ret;
}

@ockham ockham added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed in progress labels Mar 29, 2019
@ockham ockham marked this pull request as ready for review March 29, 2019 13:23
@ockham ockham requested review from a team March 29, 2019 13:23
@ockham ockham force-pushed the removew/i18n-wrapper branch from 9c94a12 to 2d67304 Compare March 29, 2019 15:47
@jetpackbot
Copy link
Collaborator

jetpackbot commented Mar 29, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against 40e357e

@simison
Copy link
Member

simison commented Apr 1, 2019

Rebased after vr block folder had been removed.

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Builds well and blocks work.

Since there have been issues with i18n and blocks in master, we might need to verify separately that translations work before 7.3 ships.

I'm seeing some translations for business hours block tho (maanantai = monday):

image

@ockham
Copy link
Contributor

ockham commented Apr 1, 2019

I'll try and produce a WP.com phab diff.

@ockham
Copy link
Contributor

ockham commented Apr 1, 2019

I'll try and produce a WP.com phab diff.

D26251-code

Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

I tested this and it worked as expected. I tried switching branches and everything continued to work. Code looks good too.

@simison simison mentioned this pull request Apr 2, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 2, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It works well for me 👍

@ockham
Copy link
Contributor

ockham commented Apr 2, 2019

Thanks everyone for reviewing!

@ockham ockham merged commit 795f8da into master Apr 2, 2019
@ockham ockham deleted the removew/i18n-wrapper branch April 2, 2019 10:17
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 2, 2019
@simison simison mentioned this pull request Apr 2, 2019
14 tasks
kraftbj added a commit that referenced this pull request Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] i18n Internationalization / i18n, adaptation to different languages [Pri] High Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup: Drop the Gutenpack i18n wrapper
8 participants