Skip to content

Conversation

@inst-danger
Copy link
Contributor

No description provided.

@github-actions
Copy link

🧪 Unit Test Results


📊 Summary

  • Total Tests: 0
  • Failed: 0
  • Skipped: 0
  • Status: ⚠️ No test results found

Last updated: Mon, 26 Jan 2026 11:00:56 GMT

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary: Korean Localization Update

Thanks for this comprehensive localization update! I've reviewed all 135 files (16,225 additions, 574 deletions) and found this to be a well-structured PR that adds complete Korean language support across the Canvas Android codebase.

Overview

  • Primary Change: Addition of Korean translations across all modules (4,053+ string resources)
  • Secondary Change: Dashboard time spent accessibility string refactoring in Horizon module
  • Minor Change: Japanese translation update in Teacher app

Issues Found

Critical Issues

  • UTF-8 BOM characters detected in all 8 new Korean locale files - Must be fixed before merge. BOM characters can cause XML parsing issues and are not consistent with Android best practices. See inline comment for affected files.

Recommendations for Verification

  • Verify corresponding code changes exist for the accessibility string refactoring (the dashboard time spent format changed from simple hours to hours+minutes with plural support). This affects 168 removed strings and 336 added strings across 43+ locales in the Horizon module.
  • Korean translation review by native speaker to verify counter words (개, 명) usage and technical terminology consistency
  • Test with TalkBack screen reader to ensure accessibility improvements work correctly
  • Verify Japanese change in apps/teacher/src/main/res/values-ja/strings.xml:23 where "tab_questions" changed from "質問" to "問題" is intentional

Positive Findings

Code Quality: Proper XML structure, consistent naming conventions, no duplicate resource names
Security: No security concerns - all changes are localization content only
Comprehensive Coverage: Full Korean translation across all modules (Teacher, Horizon, PandaRes, PandaUtils, Annotations, Canvas API, Login API, RC Editor)
Proper Encoding: HTML entities properly encoded (e.g., … for ellipsis)
Accessibility: The time spent string refactoring improves internationalization flexibility

Testing Recommendations

  • Manual testing with Korean locale on Android devices
  • Verify text rendering and character encoding
  • Test plural forms with different counts
  • Run accessibility tests with new time format strings
  • Verify XML validation passes after BOM removal

Performance Impact

Minimal - the addition of 4,053+ string resources is well within Android's capabilities and resources are loaded on-demand.

Overall Assessment: This is a solid localization PR that will significantly improve Korean language support. Once the critical BOM issue is fixed, this should be ready to merge.

@@ -0,0 +1,877 @@
<!--
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical Issue: UTF-8 BOM Character Detected

This file contains a UTF-8 Byte Order Mark (BOM) at the beginning. The BOM character () can cause:

  • XML parsing issues in some Android build tools
  • Potential runtime resource loading problems
  • Inconsistency with Android best practices (UTF-8 without BOM is preferred)

This issue affects all 8 new Korean locale files:

  • apps/teacher/src/main/res/values-ko/strings.xml
  • libs/annotations/src/main/res/values-ko/strings.xml
  • libs/canvas-api-2/src/main/res/values-ko/strings.xml
  • libs/horizon/src/main/res/values-ko/strings.xml
  • libs/login-api-2/src/main/res/values-ko/strings.xml
  • libs/pandares/src/main/res/values-ko/strings.xml
  • libs/pandautils/src/main/res/values-ko/strings.xml
  • libs/rceditor/src/main/res/values-ko/strings.xml

Fix: Remove the BOM from all files. Most text editors can save as "UTF-8 without BOM".

@github-actions
Copy link

📊 Code Coverage Report

✅ Student

  • PR Coverage: 43.42%
  • Master Coverage: 43.42%
  • Delta: +0.00%

✅ Teacher

  • PR Coverage: 25.51%
  • Master Coverage: 25.51%
  • Delta: +0.00%

✅ Pandautils

  • PR Coverage: 22.98%
  • Master Coverage: 22.98%
  • Delta: +0.00%

📈 Overall Average

  • PR Coverage: 30.64%
  • Master Coverage: 30.64%
  • Delta: +0.00%

@kristofnemere kristofnemere merged commit b8b932c into master Jan 26, 2026
29 checks passed
@kristofnemere kristofnemere deleted the translations/2026-01-26 branch January 26, 2026 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants