19701 try to come up with a solution where html tags in translations are not reliant on the order #21938
Conversation
Also implements it in the site basic page
vraja-pro
added
the
changelog: non-user-facing
|
@vraja-pro Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
|
@vraja-pro Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
|
@vraja-pro Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
|
@vraja-pro Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
Pull Request Test Coverage Report for Build 6f62ddda7c77c1b82f39a56ab18fe791914123d5Details
💛 - Coveralls |
There was a problem hiding this comment.
As you see in the comments you got, the helpers package should not be added to.
The reason for this is that we don't want to maintain a random assortment of helpers in a helper package. If there is a need for a overarching package. You could create a specific one. The smaller and targeted, the less maintenance it needs?
Why did you add it to helpers in the first place?
There was a problem hiding this comment.
I was thinking it would be easier to use in the rest of the addon/premium
There was a problem hiding this comment.
We also skipped this package when improving Jest. There are missing dev deps there.
| jest.mock( "@wordpress/element", () => ( { | ||
| createInterpolateElement: jest.fn(), | ||
| } ) ); | ||
|
|
||
| jest.mock( "@wordpress/i18n", () => ( { | ||
| sprintf: jest.fn(), | ||
| } ) ); |
There was a problem hiding this comment.
Why mock? That seems to defeat the whole purpose.
| const args = [ "world" ]; | ||
| const conversionMap = { world: <span>world</span> }; | ||
| const interpolatedElement = <span>Hello world</span>; |
There was a problem hiding this comment.
| const args = [ "world" ]; | |
| const conversionMap = { world: <span>world</span> }; | |
| const interpolatedElement = <span>Hello world</span>; | |
| const args = [ "<world />" ]; | |
| const conversionMap = { world: <span>world</span> }; | |
| const interpolatedElement = <>Hello <span>world</span></>; |
|
|
||
| expect( sprintf ).toHaveBeenCalledWith( translatedString, ...args ); | ||
| expect( createInterpolateElement ).toHaveBeenCalledWith( "Hello world", conversionMap ); | ||
| expect( result ).toBe( interpolatedElement ); |
There was a problem hiding this comment.
Then this would need to be toEqual
| const translatedString = "Hello %s"; | ||
| const args = [ "world" ]; | ||
| const conversionMap = { world: <span>world</span> }; | ||
|
|
||
| sprintf.mockImplementation( () => { | ||
| throw new Error( "Test error" ); | ||
| } ); |
There was a problem hiding this comment.
Why mock? We have the perfect use case already, as per the issue.
| const translatedString = "Hello %s"; | |
| const args = [ "world" ]; | |
| const conversionMap = { world: <span>world</span> }; | |
| sprintf.mockImplementation( () => { | |
| throw new Error( "Test error" ); | |
| } ); | |
| const translatedString = "Hello %2$sworld%1$s"; | |
| const args = [ "<world>", "</world>" ]; | |
| const conversionMap = { world: <span>world</span> }; | |
| * Create an interpolated element from a translated string and catches errors. | ||
| * | ||
| * @param {string} translatedString The translated string to be interpolated. | ||
| * @param {[string]} args The arguments to be interpolated into the translated string. |
There was a problem hiding this comment.
sprintf does not need to be string only. You can use numbers and the like with %d.
Links:
| * | ||
| * @returns {object} The interpolated element. | ||
| */ | ||
| export const i18nCreateInterpolateElement = ( translatedString, args, conversionMap ) => { |
There was a problem hiding this comment.
I think the variable naming can be improved here.
Also, the needing to write args in array form is a bit meh?
| export const i18nCreateInterpolateElement = ( translatedString, args, conversionMap ) => { | ||
| try { | ||
| return createInterpolateElement( | ||
| sprintf( |
There was a problem hiding this comment.
You changed this to include sprintf.
While making the fallback easier, it does add complexity with doing two things.
This means we no longer have the option to not sprintf when using this safer translation.
Also, if possible in the future, we could just add the HTML in the translation string? I'm not sure why we always use sprintf now. Did you investigate this?
Can you explain a bit your reasoning and thoughts here? Or rather in the issue tech choices? 😬
I would love some more details on your choices.
There was a problem hiding this comment.
The breaking error is caused when using sprintf.
There was a problem hiding this comment.
I highly doubt that, since the source includes a try/catch around it: https://github.com/WordPress/gutenberg/blob/trunk/packages/i18n/src/sprintf.js
| @@ -23,6 +23,7 @@ | |||
| "license": "GPL-3.0", | |||
| "private": false, | |||
| "dependencies": { | |||
| "@wordpress/element": "^6.14.0", | |||
There was a problem hiding this comment.
Why this version?
Our lowest support WP version is 6.5:
https://github.com/WordPress/wordpress-develop/blob/6.5.0/package.json#L113C3-L113C31
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.Documentation
Quality assurance
Innovation
innovationlabel.Fixes Try to come up with a solution where HTML tags in translations are not reliant on the order