fix: dokan js translation loading #2377
Conversation
WalkthroughThe pull request introduces significant changes focused on script dependencies and localization within the codebase. Key modifications include replacing the script dependency Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
src/utils/Mixin.js (1)
Line range hint
3-38: Consider refactoring the mixin for improved maintainability and testability.While the import change is good, there are a few suggestions to further improve this file:
The wrapper methods in the mixin (e.g.,
__,_nx,__n,sprintf) seem redundant as they simply call the imported functions without adding any functionality. Consider removing these methods and using the imported functions directly where needed in your components.The
dateTimePickerFormatmethod relies on global variables (window.dokan_get_daterange_picker_format()andwindow.dokan_helper.daterange_picker_local). This could make the code harder to test and maintain. Consider refactoring this to use dependency injection or a configuration object passed to the mixin.Here's a suggested refactoring:
import { setLocaleData, __, _x, __n, _nx, sprintf } from '@wordpress/i18n' export default { methods: { setLocaleData, dateTimePickerFormat(config = {}) { return { format: (config.getDaterangePickerFormat || (() => '')).toLowerCase(), ...(config.daterangePickerLocal || {}) } }, } }Then, where you use this mixin, you can pass the necessary configuration:
mixins: [i18nMixin], data() { return { i18nConfig: { getDaterangePickerFormat: window.dokan_get_daterange_picker_format, daterangePickerLocal: window.dokan_helper.daterange_picker_local } } }, methods: { someMethod() { const format = this.dateTimePickerFormat(this.i18nConfig); // use format... } }This approach improves testability and makes the dependencies explicit.
package.json (1)
49-50: Minor: Dependencies reorderedThe order of dependencies has been slightly modified. While this change doesn't affect functionality, it's worth noting that maintaining a consistent ordering (e.g., alphabetical) can improve readability and make future updates easier.
Consider adopting a consistent ordering strategy for all dependencies in the
package.jsonfile.webpack.config.js (1)
Line range hint
1-168: Consider the following improvements to enhance the webpack configurationWhile the current configuration is functional, there are a few areas where it could be improved:
Module consistency: The file uses a mix of CommonJS (
require) and ES6 (import) module systems. Consider standardizing to use ES6 imports throughout for better consistency.Code splitting: With multiple entry points, you might benefit from implementing code splitting to optimize bundle sizes. Consider using webpack's
SplitChunksPluginto extract common dependencies.Environment variables: Instead of using
process.env.NODE_ENVdirectly, consider using webpack'sDefinePluginto define environment variables. This can help with better dead code elimination in production builds.Optimization: For production builds, consider adding optimization configurations such as minimization and tree shaking.
Here's an example of how you could implement some of these suggestions:
const { DefinePlugin } = require('webpack'); const TerserPlugin = require('terser-webpack-plugin'); // ... (rest of the imports) module.exports = (env, argv) => { const isProduction = argv.mode === 'production'; return { // ... (rest of the config) optimization: { minimize: isProduction, minimizer: [new TerserPlugin()], splitChunks: { chunks: 'all', }, }, plugins: [ ...defaultConfig.plugins, new DefinePlugin({ 'process.env.NODE_ENV': JSON.stringify(isProduction ? 'production' : 'development'), }), // ... (rest of the plugins) ], // ... (rest of the config) }; };These changes would help optimize your build process and improve overall performance.
includes/Assets.php (1)
1086-1086: Approve addition of script translations.The addition of
wp_set_script_translationsfor each registered script is a positive change that enhances the plugin's internationalization capabilities. This ensures that all registered scripts can be properly translated.Consider moving the text domain to a constant or class property for easier maintenance:
- wp_set_script_translations( $handle, 'dokan-lite' ); + wp_set_script_translations( $handle, self::TEXT_DOMAIN );Then define
TEXT_DOMAINat the top of the class:class Assets { const TEXT_DOMAIN = 'dokan-lite'; // ... rest of the class }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- includes/Assets.php (5 hunks)
- package.json (1 hunks)
- src/utils/Mixin.js (1 hunks)
- webpack.config.js (1 hunks)
Additional comments not posted (7)
package.json (1)
52-53: Addition of @wordpress/i18n dependencyThe addition of the
@wordpress/i18npackage aligns with the PR's objective of fixing JavaScript translation loading issues. This package provides internationalization utilities for WordPress projects.To ensure this addition is properly utilized, let's verify its usage in the project:
webpack.config.js (3)
Line range hint
1-168: Summary of webpack.config.js review
- The main change (adding
...defaultConfig.plugins) has been approved as it follows good practices for including default configurations.- Several suggestions for improving the overall webpack configuration have been provided, including standardizing module imports, implementing code splitting, and optimizing for production builds.
- A clarification has been requested regarding how this change relates to the PR's stated objective of fixing JavaScript translation loading issues.
Overall, the configuration appears to be functional, but there's room for optimization and clarity regarding its role in addressing the PR's objectives.
84-84: Clarify the relationship between this change and the PR objectivesThe PR objectives mention fixing issues related to JavaScript translation loading in the Dokan plugin. However, the change in this file (adding
...defaultConfig.plugins) doesn't seem to directly address translation loading.Could you please clarify how this webpack configuration change relates to the stated objective of fixing JavaScript translation loading? Are there any specific plugins in
defaultConfig.pluginsthat are crucial for translation functionality?To help understand the context better, let's check for any translation-related configurations:
#!/bin/bash # Description: Search for translation-related configurations # Test: Look for any translation or internationalization related imports or configurations rg --type javascript "i18n|translation|localization|gettext"This will help us identify if there are any translation-related configurations that might be affected by this change.
84-84: Approve the addition of default pluginsThe change to include
...defaultConfig.pluginsin thepluginsarray is a good practice. It ensures that all default plugins from the WordPress scripts configuration are included alongside any custom plugins defined in this configuration.To ensure this change doesn't introduce any conflicts or issues, let's verify the contents of
defaultConfig.plugins:This will help us understand the default plugins being included and ensure there are no conflicts with the custom plugins defined in this file.
includes/Assets.php (3)
Line range hint
1-1186: Summary of Asset Management ChangesThe modifications in this file represent a significant shift in how Dokan handles internationalization:
- Replaced
dokan-i18n-jedwithwp-i18nacross multiple script dependencies.- Removed the
dokan-i18n-jedscript registration.- Added
wp_set_script_translationsfor all registered scripts.- Updated the localization target from
dokan-i18n-jedtodokan-vue-bootstrap.These changes collectively improve the plugin's compatibility with WordPress core internationalization functions and enhance its translation capabilities. The consistent application of these changes across the file indicates a well-planned approach to updating the asset management system.
To ensure a smooth transition, please consider the following action items:
- Run the suggested verification scripts to catch any missed references to
dokan-i18n-jed.- Update any documentation or developer guidelines to reflect the new internationalization approach.
- Thoroughly test the plugin's translation functionality in various languages to confirm that these changes have not introduced any regressions.
Line range hint
475-516: Approve consistent updates to script dependencies.The addition of
'wp-i18n'to multiple script dependencies is consistent with the earlier changes and reinforces the shift towards using WordPress core internationalization functions. This uniform approach across various Dokan scripts is a positive change.To ensure all necessary scripts have been updated, please run the following script to check for any Dokan scripts that might still be missing the
wp-i18ndependency:#!/bin/bash # Search for Dokan script registrations without 'wp-i18n' dependency echo "Searching for Dokan scripts without 'wp-i18n' dependency:" rg --type php "wp_enqueue_script\(\s*['\"]\w+['\"]" | rg -v "wp-i18n"
638-638: Approve update to script localization target.The change from
dokan-i18n-jedtodokan-vue-bootstrapfor script localization is consistent with the earlier modifications to script dependencies. This ensures that the localized data is attached to the correct script after the removal ofdokan-i18n-jed.To ensure the localized data is correctly attached and accessible, please run the following script to check for any remaining references to
dokan-i18n-jedin localization calls:
| @@ -1,4 +1,4 @@ | |||
| import { setLocaleData, __, _x, __n, _nx, sprintf } from '@/utils/i18n' | |||
| import { setLocaleData, __, _x, __n, _nx, sprintf } from '@wordpress/i18n' | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Import Path Successfully Updated. Review src/utils/i18n.js for Redundancy.
The shell script confirms that there are no remaining imports from @/utils/i18n, and all usages of the internationalization functions are now correctly referencing @wordpress/i18n. However, the src/utils/i18n.js file is still present in the codebase.
- Action Items:
- Verify if
src/utils/i18n.jsis used elsewhere in the project. - If unused, consider removing it to maintain a clean and efficient codebase.
- Verify if
Analysis chain
LGTM! Verify consistency across the codebase.
The change from '@/utils/i18n' to '@wordpress/i18n' aligns with the PR objective of fixing JavaScript translation loading. This shift to using WordPress's built-in i18n functions is a good practice for maintaining consistency with WordPress standards.
To ensure this change is consistent across the codebase and doesn't introduce any breaking changes, please run the following verification script:
Additionally, please ensure comprehensive testing is performed to verify that all internationalization functions continue to work as expected after this change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports from '@/utils/i18n' and usages of the imported functions
# Test 1: Check for any remaining imports from '@/utils/i18n'
echo "Checking for remaining imports from '@/utils/i18n':"
rg --type js "from '@/utils/i18n'" src
# Test 2: Check for usages of the imported functions
echo "Checking for usages of imported i18n functions:"
rg --type js -w "setLocaleData|__|_x|__n|_nx|sprintf" src
# Note: Review the output to ensure all occurrences are using '@wordpress/i18n'
Length of output: 73419
|
WPML doesn’t automatically translate strings inside JavaScript files. The current issue is that WPML cannot generate the necessary JSON translation files for JavaScript strings, even though WordPress has a built-in function wp_set_script_translations() to handle this. Refrence : https://wpml.org/errata/translating-strings-from-javascript-files-using-wp_set_script_translations/ |
osmansufy
added
Needs: Testing
Migration from dokan-i18n-jed to wp-i18nOverviewThis PR updates the Dokan plugin to use WordPress's built-in Key Changes
Implementation Details
Future Considerations
Developer Notes
Testing
By adopting |
WPML JavaScript Translation IssueOverview of the ProblemWPML (WordPress Multilingual Plugin) currently has a limitation in its handling of JavaScript translations. While WPML successfully translates strings in PHP files, it does not automatically generate the necessary JSON files for translating strings in JavaScript files. Detailed Description
ReferenceThis issue is documented in WPML's official errata: https://wpml.org/errata/woocommerce-checkout-block-fails-to-translate-some-values/ Impact
Current WorkaroundUntil WPML addresses this issue, Loco Translate: An Alternative Solution: Unlike WPML, Loco Translate does support the creation of JSON files for JavaScript translations. This makes it a viable alternative for projects that heavily rely on JavaScript and require multilingual support.
Long-term SolutionThe permanent solution to this issue requires action from the WPML development team:
|
mrabbani
added
👍 Dev Review Done
and removed
Needs: Dev Review
StalinDurjo
added
QA approved
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores