[I18n] Fix issues and add improvements to JavaScript internationalization#2034
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing internationalization and user interface functionality. Key modifications include the addition of methods in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (6)
js/admin/embed.js (2)
132-138: LGTM: Consistent use of sprintf for string formattingThe string formatting in the
getModalOptionsfunction has been updated to usesprintfconsistently. This change improves the handling of translations with placeholders and aligns with WordPress internationalization best practices.Consider extracting the repeated
sprintf(__(...), typeDescription)pattern into a helper function to reduce code duplication and improve maintainability. For example:function formatWithType(string) { return sprintf(__(string, 'formidable'), typeDescription); } // Usage const existingPageDescription = formatWithType('Embed your %s into an existing page.');
Line range hint
1-516: Overall assessment: Improved internationalization and consistencyThe changes in this file consistently update string formatting to use
sprintffromwp.i18n. These modifications align with WordPress internationalization best practices, improving the handling of translations with placeholders throughout the code. The updates also enhance code consistency and potentially improve accessibility, particularly in the case of the aria-label formatting.To further improve the code:
- Consider extracting repeated
sprintfpatterns into helper functions to reduce duplication.- Ensure that all translatable strings in the file have been updated to use this new formatting approach for consistency.
- If not already done, consider adding translator comments to provide context for these strings, which can aid in accurate translations.
js/admin/applications.js (2)
213-213: LGTM: Improved internationalization usingsprintfThe use of
sprintfhere improves the internationalization of the string. This change allows for better handling of pluralization and positioning of variables in different languages.Consider adding a translator comment to provide context for translators:
+ // translators: %d: Number of application templates. return sprintf( __( 'All Items (%d)', 'formidable' ), state.templates.length );
377-378: LGTM: Improved accessibility with internationalizedaria-descriptionThe use of
sprintffor thearia-descriptionattribute improves accessibility and internationalization. This change allows for better positioning of the template name in different languages.Consider adding a translator comment to provide context for translators:
+ // translators: %s: Application Template Name const ariaDescription = sprintf( __( '%s Template', 'formidable' ), data.name );classes/controllers/FrmAppController.php (2)
Line range hint
28-51: Avoid using anonymous functions in hooks to enhance maintainabilityIn
maybe_add_black_friday_submenu_item(), an anonymous function is used as a callback foradd_action(). This can make it difficult for other developers to remove or modify this action if needed because the function is not easily referenceable. Consider defining a named callback method instead.Apply this diff to refactor the code:
+private static function add_black_friday_submenu() { + $is_black_friday = self::is_black_friday(); + $is_cyber_monday = self::is_cyber_monday(); + + if ( ! $is_black_friday && ! $is_cyber_monday ) { + return; + } + + $black_friday_menu_label = $is_black_friday ? __( 'Black Friday!', 'formidable' ) : __( 'Cyber Monday!', 'formidable' ); + $black_friday_menu_label = '<span class="frm-orange-text">' . esc_html( $black_friday_menu_label ) . '</span>'; + + add_submenu_page( 'formidable', 'Formidable', $black_friday_menu_label, 'frm_change_settings', 'formidable-black-friday', array( __CLASS__, 'redirect_blackfriday' ) ); +} + -add_action( - 'admin_menu', - function () use ( $black_friday_menu_label ) { - add_submenu_page( 'formidable', 'Formidable', $black_friday_menu_label, 'frm_change_settings', 'formidable-black-friday', 'FrmAppController::redirect_blackfriday' ); - }, - 1000 -); +add_action( 'admin_menu', array( __CLASS__, 'add_black_friday_submenu' ), 1000 );
Line range hint
53-63: Make sale date ranges configurable for easier maintenanceThe methods
is_black_friday()andis_cyber_monday()have hardcoded date ranges. To enhance flexibility and reduce the need for code changes each year, consider defining these date ranges as class constants or retrieving them from a configuration file or database.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- classes/controllers/FrmAddonsController.php (1 hunks)
- classes/controllers/FrmAppController.php (1 hunks)
- classes/models/FrmReviews.php (1 hunks)
- js/admin/applications.js (4 hunks)
- js/admin/embed.js (6 hunks)
- js/admin/style.js (3 hunks)
- js/formidable_admin.js (3 hunks)
- stripe/helpers/FrmTransLiteListHelper.php (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- classes/models/FrmReviews.php
🧰 Additional context used
🔇 Additional comments (21)
js/admin/embed.js (5)
8-8: LGTM: Import statement updated correctlyThe import statement has been updated to include
sprintfalong with__fromwp.i18n. This change is necessary to support the new string formatting approach and follows good practices by destructuring the imports.
77-77: LGTM: Improved string formatting with sprintfThe string formatting has been updated to use
sprintf, which is more consistent with WordPress internationalization best practices. This change improves code readability and makes it easier to handle translations with placeholders.
181-181: LGTM: Consistent use of sprintf in addExistingPageDropdownThe string formatting in the
addExistingPageDropdownfunction has been updated to usesprintf, maintaining consistency with the other changes in the file. This improves the handling of translations with placeholders.
192-192: LGTM: Button text formatting updated consistentlyThe button text formatting in the
addExistingPageDropdownfunction has been updated to usesprintf, maintaining consistency with the other changes in the file. This improves the handling of translations with placeholders for the button text.
451-451: LGTM: Improved accessibility with formatted aria-labelThe
aria-labelformatting in thegetCopyIconfunction has been updated to usesprintf, consistent with other changes. This improvement not only maintains consistency in internationalization but also enhances accessibility by providing a properly formatted label for screen readers.stripe/helpers/FrmTransLiteListHelper.php (1)
142-144: Improved internationalization and translation flexibility.The changes in this section enhance the internationalization capabilities:
- A translator comment has been added, providing context for the string.
- The
sprintffunction now uses four placeholders instead of three, separating the closing span tag.These modifications allow for more flexibility in translations where the order of elements might need to change, which is a good practice for internationalization.
js/admin/applications.js (3)
8-8: LGTM: Import ofsprintfaddedThe addition of
sprintfto the import statement is appropriate and aligns with the PR objective of improving string formatting. This change sets the foundation for the subsequent improvements in internationalization throughout the file.
422-423: LGTM: Improved internationalization of warning messageThe use of
sprintffor the warning message improves internationalization. This change allows for better positioning of the required plan name in different languages. The existing translator comment provides good context for translators.
Line range hint
1-524: Overall improvements in internationalization and string formattingThe changes in this file consistently improve internationalization and string formatting by replacing string concatenation with
sprintf. These modifications align well with the PR objectives and address the issues mentioned in the linked issue #2033.Key improvements:
- Addition of
sprintfimport- Use of
sprintffor better handling of variable positioning in translated strings- Improved accessibility with internationalized
aria-descriptionThese changes will enhance the plugin's compatibility with different languages and improve the overall user experience for non-English speakers.
classes/controllers/FrmAddonsController.php (1)
72-72: LGTM! This change implements the missing script translations.The addition of
wp_set_script_translations( self::SCRIPT_HANDLE, 'formidable' );correctly implements internationalization for the script, addressing one of the main objectives of this PR. This change follows WordPress best practices for script translation and will allow for proper localization of the add-ons page.js/admin/style.js (4)
13-13: Update import statement to use object destructuring.The import statement has been updated to include
sprintfalong with__. This change allows for better string formatting in internationalized messages.
155-156: Improved string formatting for internationalization.The code now uses
sprintffor formatting the "Show all" text with the number of hidden items. This change enhances readability and makes it easier to translate the string with a dynamic number.
Line range hint
1034-1067: New function to handle select placeholder colors.This new
setSelectPlaceholderColorfunction improves the user interface by dynamically setting the color of select placeholders. It uses modern JavaScript features likequerySelectorAll,forEach, and arrow functions. The function also adds event listeners to handle future changes, which is a good practice for maintaining consistent behavior.
Line range hint
1-1073: Overall improvements to style handling and internationalization.This update to the
style.jsfile includes several enhancements:
- Improved internationalization support with the addition of
sprintffor better string formatting.- Enhanced pagination handling for style cards.
- New functionality for managing select placeholder colors.
- Various minor improvements and code modernization throughout the file.
These changes contribute to a better user experience and more maintainable code.
classes/controllers/FrmAppController.php (3)
Line range hint
14-14: Properly integrated Black Friday submenu item into the menuAdding
self::maybe_add_black_friday_submenu_item();in themenu()method ensures that the promotional submenu item is conditionally added during the sale period. The implementation is correct and aligns with the intended functionality.
822-822: Internationalization: Set script translations for 'formidable_admin'The use of
wp_set_script_translations( 'formidable_admin', 'formidable' );correctly prepares the 'formidable_admin' script for translation, ensuring that localized strings are properly loaded.
824-824: Internationalization: Set script translations for 'formidable_embed'Similarly,
wp_set_script_translations( 'formidable_embed', 'formidable' );ensures that the 'formidable_embed' script can load the appropriate translation files, aligning with best practices for internationalization.js/formidable_admin.js (4)
3643-3645: Looks good!The function logic is correct and the implementation accurately generates the confirmation message using
sprintf().
6113-6114:⚠️ Potential issueMinor typo in the error message.
There is a minor typo in the error message where "Duplicate" is misspelled as "Duplciate".
- /* translators: %s: The detected option value. */ + /* translators: %s: The detected duplicate option value. */Likely invalid or redundant comment.
6855-6856:⚠️ Potential issueTypo in the translator comment.
There is a typo in the translator comment. It should say "are not installed" instead of "is not installed".
- /* translators: %s: Form Setting section name (ie Form Permissions, Form Scheduling). */ + /* translators: %s: Form Setting section name (e.g. Form Permissions, Form Scheduling). */Likely invalid or redundant comment.
Line range hint
3647-3662: Verify the impact of deleting a section on the entire form.The code looks fine for deleting a section. However, deleting a section is a significant change that can impact the entire form structure and any conditional logic or calculations that depend on fields inside the section.
I recommend adding a verification step before deleting the section:
If the script returns any results, display a warning to the user about the potential impact and confirm they still want to proceed with the deletion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
js/formidable_admin.js (3)
3643-3643: Nitpick: Add a space after the opening curly brace.For consistency with the existing code style, add a space after the opening curly brace.
confirmFieldsDeleteMessage( numberOfFields ) {
6113-6114: Nitpick: Add a space after the opening comment tag.For consistency with the existing code style, add a space after the opening
/*in the comment./* translators: %s: The detected option value. */
6855-6855: Nitpick: Add a space after the opening comment tag.For consistency with the existing code style, add a space after the opening
/*in the comment./* translators: %s: Form Setting section name (ie Form Permissions, Form Scheduling). */
2 participants
Hi,
This PR improves the JavaScript i18n of the plugin with the below items:
wp_set_script_translations()).sprintf()instead of .replace().Here is a screenshot of the dropdown as currently is:
Here is a screenshot after adding
wp_set_script_translations( 'formidable_admin', 'formidable' );Fixes #2033