-
Notifications
You must be signed in to change notification settings - Fork 219
Add textdomain validation to .eslintrc.js #5021
Add textdomain validation to .eslintrc.js #5021
Conversation
Size Change: +8.37 kB (+1%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
Nice, I see it's working already in the diff. I'm not sure on the answer for For Stripe, I'm not actually sure why we still bundle this. Let's check with @nerrad -- can we remove Stripe from Blocks now it's integrated in the extension? |
It was left to cover folks that might not have upgraded the Stripe extension with the integration but were keeping blocks up to date. I think there's been enough time that we could feasibly remove it, but we maybe could do some version checking to verify the impact would be minimal. I'll do that and get back here (or create an issue if we can). |
I think we can remove that. |
I removed
Shall we ignore this for now and open this PR for review, @nerrad. Or do you want to clarify the Stripe integration first? |
The Stripe integration conversation doesn't need to block this PR. |
@nerrad Thanks for letting me know. In this case, I'll open the PR up for review. 😀 |
@mikejolley Feel free to review this PR when you have time doing so. In addition to the original PR, I removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻 Very useful. Good job, this will save headaches.
Will you be creating a follow up for all the eslint violations it's flagged below?
Thanks, @mikejolley! 🙌
Sure. I'll create a new issue and a fix for that, given that this issue is rather small. I'll link the new issue to this PR then. |
* Add textdomain validation to .eslintrc.js * Only allow JS textdomain woo-gutenberg-products-block
* Add textdomain validation to .eslintrc.js * Only allow JS textdomain woo-gutenberg-products-block
Discovered in #5005 and #5020
Note
At the moment, we only validate the textdomain within PHP files based on the following PHPCS rule:
https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/888760656a13e83562fc1e995822043c931fe339/phpcs.xml#L18-L22
However, we do not validate the textdomain within JS, TS and TSX files. This PR aims to add textdomain validation for these files.
The solution, I used, is coming from Gutenberg. The following pages provide more information:
In my solution, I used the textdomains
default
andwoo-gutenberg-products-block
.woo-gutenberg-products-block
validates our plugin-related textdomain, whiledefault
allows having strings without textdomain, such as default WordPress elements which get their translation directly from core. An example for such a string can be found here:https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/20389f280acd3663f22eac3a1c0b132a64e9fd0c/assets/js/blocks/handpicked-products/block.js#L168-L178
Demo output
Output when allowing an empty textdomain
Output when not allowing an empty textdomain
Benefits
In case a change contains an incorrect textdomain, this problem then gets automatically detected as seen on https://github.com/woocommerce/woocommerce-gutenberg-products-block/pull/5021/files.
Questions
Shall we allow the use of an empty textdomain?
As mentioned before, there are some sections in which we're not using a textdomain. However, these strings are still listed on WordPress.org and can be translated there. This might lead to confusion if a string is translated there, but this translation does not appear in our plugin.
Shall we allow the use of external textdomains?
Similar to the previous question, some sections use the textdomain
woocommerce-gateway-stripe
. While these sections are related to the WooCommerce Stripe extension, if the upstream changes one of these strings, then it's no longer translated. Furthermore, as mentioned in the question before, it can be confusing that the strings are editable on WordPress.org, but will not show up within our plugin due to the different textdomain.