refactor: remove i18n variable in editor instance #183
Conversation
WalkthroughThe pull request introduces a comprehensive refactoring of language text retrieval across multiple components in the Fluent Editor. The primary change involves replacing direct access to Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
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 (
|
WalkthroughThis pull request refactors the Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
packages/fluent-editor/src/table/modules/table-operation-menu.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/fluent-editor/src/custom-clipboard.ts (2)
157-157: Consider usingtextContentor ensuring content is sanitized
UsinginnerHTMLhere is acceptable ifthis.quill.getLangText('pasting')is always trusted (e.g. from a safe dictionary). Otherwise, you might consider usingtextContentto avoid any potential HTML injection.
474-474: Avoid relying on string comparison for placeholder logic
Currently, the logic picks the placeholder image based onthis.quill.getLangText('img-error') === 'Image Copy Error'. This approach can break if the text is changed or localized differently in the future. A more robust strategy might be to store placeholders in your language config and retrieve them accordingly, or switch using the current language code.packages/fluent-editor/src/fluent-editor.ts (1)
45-49: NewgetLangTextmethod
This is a good step for ensuring safe fallback behavior when the i18n module is missing. However, if the key is not found in the dictionary, consider logging or returning a default text to avoid undefined outputs.
🛑 Comments failed to post (1)
packages/fluent-editor/src/table/modules/table-operation-menu.ts (1)
69-69:
⚠️ Potential issuePotential leftover call to
linkplaceholder.
It seems inconsistent to switch all references togetLangTextexcept for this line. Iflinkplaceholderis intentional, please clarify its role; otherwise, consider usinggetLangTextfor language consistency.- this.DEFAULT_COLOR_SUBTITLE = this.quill.linkplaceholder('sub-title-bg-color') + this.DEFAULT_COLOR_SUBTITLE = this.quill.getLangText('sub-title-bg-color')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.this.DEFAULT_COLOR_SUBTITLE = this.quill.getLangText('sub-title-bg-color')
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/fluent-editor/src/fluent-editor.ts (2)
36-39: Consider improving type safety of the i18n module retrieval.The getter implementation correctly encapsulates the language access and provides a good fallback mechanism. However, the type casting could be made more explicit for better type safety.
Consider this improvement:
- const i18nModule = this.getModule('i18n') as I18N + const i18nModule = this.getModule('i18n') as I18N | nullThis makes it clearer that
getModulemight return null, matching the null check in the implementation.
45-49: Enhance type safety and documentation of getLangText method.The implementation provides a good mechanism for language text retrieval with proper fallback. However, there are several opportunities for improvement:
Consider these enhancements:
- getLangText(name: string) { + /** + * Retrieves localized text for the given key. + * @param name - The key to lookup in the language configuration + * @returns The localized text string + */ + getLangText(name: string): string { - const i18nModule = this.getModule('i18n') as I18N + const i18nModule = this.getModule('i18n') as I18N | null if (!i18nModule) return LANG_CONF[defaultLanguage][name] - return i18nModule.options.langText[name] + return i18nModule.options.langText?.[name] ?? LANG_CONF[defaultLanguage][name] }These changes:
- Add JSDoc documentation
- Specify return type
- Improve type safety of module retrieval
- Add null safety for langText access with a fallback
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/fluent-editor/src/counter/index.ts(3 hunks)packages/fluent-editor/src/custom-clipboard.ts(2 hunks)packages/fluent-editor/src/fluent-editor.ts(2 hunks)packages/fluent-editor/src/i18n/index.ts(0 hunks)packages/fluent-editor/src/link/modules/tooltip.ts(1 hunks)packages/fluent-editor/src/table/modules/table-operation-menu.ts(11 hunks)packages/fluent-editor/src/toolbar/toolbar-tip.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/fluent-editor/src/i18n/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/fluent-editor/src/toolbar/toolbar-tip.ts
- packages/fluent-editor/src/custom-clipboard.ts
- packages/fluent-editor/src/counter/index.ts
- packages/fluent-editor/src/link/modules/tooltip.ts
- packages/fluent-editor/src/table/modules/table-operation-menu.ts
🔇 Additional comments (2)
packages/fluent-editor/src/fluent-editor.ts (2)
6-6: LGTM: Import changes align with the new language handling implementation.
The additional imports for defaultLanguage and LANG_CONF are necessary for the refactored language text retrieval mechanism.
35-49: Verify the impact of language handling changes.
The refactoring successfully achieves the PR objectives by:
- Making
langreadonly through a getter - Replacing
langTextvariable withgetLangTextmethod
Let's verify that all consumers of these language-related APIs have been updated accordingly.
✅ Verification successful
Let me check the specific usage in the i18n module to ensure all changes are properly aligned.
The language handling changes are correctly implemented
The usage of langText and lang in i18n/index.ts is correct and doesn't need modifications because:
- The
langTextaccess is within the I18N module itself, which owns and manages these properties - The
langassignments are part of the internal language management logic of the I18N module - External access is properly handled through the new getter and method in
fluent-editor.ts
The refactoring successfully encapsulates the language properties while maintaining the necessary internal access in the I18N module. No further changes are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct access to langText that needs to be updated
rg -l "\.langText\b" --type ts
# Search for direct language property assignments that should now use the getter
rg -l "\.lang\s*=" --type ts
Length of output: 189
Script:
#!/bin/bash
# Check the context of langText usage in i18n module
rg "\.langText\b" packages/fluent-editor/src/i18n/index.ts -B 2 -A 2
# Check the context of lang assignments in i18n module
rg "\.lang\s*=" packages/fluent-editor/src/i18n/index.ts -B 2 -A 2
Length of output: 1223
|
@kagol test是不是有问题？ |
加了一个 PeterCat 答疑机器人之后，控制台会报错，只要控制台报错了，就会导致测试用例失败，富文本编辑器本身的功能是没问题的。 |
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
make
langreadonly in quill. changelangTexttogetLangTextfunctionDoes this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
Language & Localization
Clipboard Improvements
Bug Fixes