Make highlighter controls translatable - #1261
Conversation
Summary of ChangesHello @SteveJonesDev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the internationalization capabilities of the frontend highlighter component. By making key UI elements like navigation buttons and documentation links translatable, and by refactoring the issue summary display to properly handle localized string formatting and pluralization, the changes ensure a more accessible and user-friendly experience for a global audience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR internationalizes front-end highlighter strings in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Potential focus areas:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.js📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/**/*📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-08-29T11:15:35.151ZApplied to files:
📚 Learning: 2025-08-29T11:15:35.151ZApplied to files:
📚 Learning: 2025-08-29T11:15:35.151ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully adds internationalization support for several UI elements in the highlighter, including navigation buttons and the documentation link. It also refactors the issue summary generation to use translatable strings.
My review identifies a critical logic issue in the new summary generation. The implementation incorrectly includes issue types with a count of zero in the summary message, leading to confusing output for the user. I've provided a code suggestion to fix this by conditionally building the summary parts.
| const summaryParts = [ | ||
| sprintf( _n( '%1$s error', '%1$s errors', errorCount, 'accessibility-checker' ), errorCount ), | ||
| sprintf( _n( '%1$s warning', '%1$s warnings', warningCount, 'accessibility-checker' ), warningCount ), | ||
| sprintf( _n( '%1$s ignored issue', '%1$s ignored issues', ignoredCount, 'accessibility-checker' ), ignoredCount ), | ||
| ]; | ||
|
|
||
| switch ( summaryParts.length ) { | ||
| case 1: | ||
| textContent = sprintf( __( '%1$s detected.', 'accessibility-checker' ), summaryParts[ 0 ] ); | ||
| break; | ||
| case 2: | ||
| textContent = sprintf( __( '%1$s and %2$s detected.', 'accessibility-checker' ), summaryParts[ 0 ], summaryParts[ 1 ] ); | ||
| break; | ||
| default: | ||
| textContent = sprintf( __( '%1$s, %2$s, and %3$s detected.', 'accessibility-checker' ), summaryParts[ 0 ], summaryParts[ 1 ], summaryParts[ 2 ] ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
The current logic for building summaryParts is flawed. The array is unconditionally populated with three items, regardless of whether their counts are zero. This causes the switch statement to always fall into the default case, producing a message that includes zero-count items, like "1 error, 0 warnings, and 0 ignored issues detected."
To fix this, summaryParts should be populated conditionally, only adding parts where the count is greater than zero. This ensures the switch statement correctly formats the summary string based on the actual number of issue types found.
const summaryParts = [];
if ( errorCount > 0 ) {
summaryParts.push( sprintf( _n( '%1$s error', '%1$s errors', errorCount, 'accessibility-checker' ), errorCount ) );
}
if ( warningCount > 0 ) {
summaryParts.push( sprintf( _n( '%1$s warning', '%1$s warnings', warningCount, 'accessibility-checker' ), warningCount ) );
}
if ( ignoredCount > 0 ) {
summaryParts.push( sprintf( _n( '%1$s ignored issue', '%1$s ignored issues', ignoredCount, 'accessibility-checker' ), ignoredCount ) );
}
switch ( summaryParts.length ) {
case 1:
textContent = sprintf( __( '%1$s detected.', 'accessibility-checker' ), summaryParts[ 0 ] );
break;
case 2:
textContent = sprintf( __( '%1$s and %2$s detected.', 'accessibility-checker' ), summaryParts[ 0 ], summaryParts[ 1 ] );
break;
case 3:
textContent = sprintf( __( '%1$s, %2$s, and %3$s detected.', 'accessibility-checker' ), summaryParts[ 0 ], summaryParts[ 1 ], summaryParts[ 2 ] );
break;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontendHighlighterApp/index.js (1)
289-289: Consider internationalizing remaining user-facing strings.Several other user-facing strings throughout the file are not yet using
wp.i18nfor translation:
- Line 289:
aria-labelwith template literal- Lines 408, 410, 416, 417, 418: Panel UI labels ("Accessibility Checker Tools", "Close", "Accessibility Checker", "Loading...")
- Lines 524, 530, 534: Status messages ("The element is not visible...", etc.)
- Line 710: "Fix Issue" button text
- Line 1032: "Landmark:" label
While these are outside the current PR scope, the coding guidelines require all user-facing JavaScript strings to use
wp.i18nfor translation.As per coding guidelines.
Also applies to: 408-408, 410-410, 416-416, 417-417, 418-418, 524-524, 530-530, 534-534, 710-710, 1032-1032
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/frontendHighlighterApp/index.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All user-facing JavaScript strings must use wp.i18n for translation
Files:
src/frontendHighlighterApp/index.js
src/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Store source frontend assets (JS/CSS) in /src; do not commit edits directly to built files
Files:
src/frontendHighlighterApp/index.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: equalizedigital/accessibility-checker#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-29T11:15:35.151Z
Learning: Applies to **/*.js : All user-facing JavaScript strings must use wp.i18n for translation
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP 6.2
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
🔇 Additional comments (3)
src/frontendHighlighterApp/index.js (3)
7-7: LGTM! Import statement is correct.The addition of
sprintfto the existing@wordpress/i18nimports is necessary for formatting the localized issue count messages.
421-422: LGTM! Button labels properly internationalized.The Previous and Next button labels are correctly wrapped with the
__()function using the proper text domain.
716-716: LGTM! Documentation link properly internationalized.The "Full Documentation" text is correctly wrapped with the
__()function.
pattonwebz
left a comment
pattonwebz
left a comment
There was a problem hiding this comment.
The translations here seem good to me. It's much nicer seeing them using sprintf for positional replacements instead of just inline string literals.
I did have 2 comments about some logic changes around counting error, warning, and ignore, and about how readable the switch statement added for those changes is.
| // Remove the trailing comma and add "detected." | ||
| textContent = textContent.slice( 0, -2 ) + ' ' + __( 'detected.', 'accessibility-checker' ); | ||
|
|
||
| if ( ignoredCount > 0 ) { |
There was a problem hiding this comment.
This here and the error and warning count checks were changed from greater than or equal to 0 to be just greater than 0. Is that intentional so that the messages don't output if there's 0 items?
| switch ( summaryParts.length ) { | ||
| case 1: | ||
| textContent = sprintf( __( '%1$s detected.', 'accessibility-checker' ), summaryParts[ 0 ] ); | ||
| break; | ||
| case 2: | ||
| textContent = sprintf( __( '%1$s and %2$s detected.', 'accessibility-checker' ), summaryParts[ 0 ], summaryParts[ 1 ] ); | ||
| break; | ||
| default: | ||
| textContent = sprintf( __( '%1$s, %2$s, and %3$s detected.', 'accessibility-checker' ), summaryParts[ 0 ], summaryParts[ 1 ], summaryParts[ 2 ] ); | ||
| break; |
There was a problem hiding this comment.
For easier parsing, I would prefer that there be a case covering 3 here. It could just defer to the default but it wasn't immediately clear to me why there wasn't a 3 and the default assumed it had 3 length.
2 participants
This pull request improves internationalization and accessibility for the frontend highlighter app by ensuring all user-facing text is properly localized using WordPress i18n functions. The changes replace hardcoded strings with calls to translation functions, making the interface fully translatable and more user-friendly for non-English users.
Internationalization and Localization Improvements:
__,_n, andsprintffunctions from@wordpress/i18nfor proper localization throughoutsrc/frontendHighlighterApp/index.js. [1] [2] [3] [4] [5] [6]Accessibility and UI Enhancements:
These updates make the app more maintainable and accessible to a global audience.
https://linear.app/equalize-digital/issue/PRO-344/some-missing-translations-in-front-end-highlighter-and-open-issues
Fixes: https://linear.app/equalize-digital/issue/PRO-344/some-missing-translations-in-front-end-highlighter-and-open-issues
Summary by CodeRabbit