feat: remove header-list and toolbar-tip internal dependencies #178
Conversation
WalkthroughThis pull request introduces several enhancements to the Fluent Editor documentation and core functionality. Key changes include the addition of a new sidebar menu item for '工具栏提示', updates to toolbar configurations, the introduction of a new Vue component for toolbar tips, and modifications to dependency management. The overall structure of the editor's module registration and internationalization approach has been streamlined, focusing on improving plugin integration and user guidance. Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
github-actions
bot
added
the
documentation
WalkthroughThis pull request removes internal dependencies on Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
zzxming
force-pushed
the
chore-module-extract
branch
from
ee699aa to
36e3f95
Compare
![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 (13)
packages/fluent-editor/src/config/types/editor-config.interface.ts (1)
17-20: Preserve explicit types for clarity.Dropping the explicit
Filetype can reduce clarity. Keeping it ensures that callers don't inadvertently pass incompatible objects.imageUpload?: ({ file, callback, editor }: { file: File; callback: Function; editor: any }) => void ... fileUpload?: ({ file, callback, editor }: { file: File; callback: Function; editor: any }) => voidpackages/fluent-editor/src/i18n/index.ts (2)
11-15: Remove unusedisFullscreenif not needed.
isFullscreenis declared but never used. Remove to streamline the class definition unless you plan to add relevant functionality later.-export class I18N { - isFullscreen: boolean = false - options: I18NOptions = { +export class I18N { + options: I18NOptions = { lang: 'en-US', langText: LANG_CONF['en-US'], }
17-19: Double-checkObject.assignusage order.
Object.assign({}, options, this.resolveLanguageOption(options || {}))merges user-provided options before the resolved defaults. Generally, you want defaults first and user overrides last.- this.options = Object.assign({}, options, this.resolveLanguageOption(options || {})) + this.options = Object.assign({}, this.resolveLanguageOption(options || {}), options)packages/docs/fluent-editor/demos/toolbar-tip.vue (1)
17-19: Consider avoiding direct assignment to global objects.
Assigninghljs,katex, orHtml2Canvasto thewindowobject can pollute global scope, making it more difficult to maintain or debug. A more localized import or a dedicated plugin might be preferable.packages/fluent-editor/src/counter/index.ts (1)
36-37: User-facing language text.
Selecting the correct label forcharvs.wordis crucial for accurate user feedback. This logic is straightforward and appears correct, but consider adding more descriptive labels if your user base has broader language needs.packages/fluent-editor/src/table/formats/header.ts (1)
6-8: ExtendingOriginHeadereffectively customizes Quill’s built-in header blot.
This aligns your code with Quill’s internal approach. Just confirm that dynamic attributes introduced (data-xxx) do not break other Quill modules.packages/fluent-editor/src/link/modules/tooltip.ts (1)
50-50: Gracefully handle missinglangTextentries.Using optional chaining for
this.quill.langText?.linkplaceholderis good. However, consider a fallback or default string iflangTextorlinkplaceholderis undefined, preventing potential user confusion or blank placeholders in exceptional cases.packages/fluent-editor/src/custom-clipboard.ts (2)
157-157: Handle missing or empty translation safely.Updating the clipboard container with
this.quill.langText.pastingis correct, but consider verifying this string to ensure a default or localized fallback if it's missing or empty. This prevents placeholder values from appearing in production or leading to runtime errors if undefined.
474-474: Provide a more descriptive placeholder for image copy errors.Using
this.quill.langText['img-error']fosters better i18n but be sure it is user-friendly and highlights the nature of the error. Include best practices for user-friendly and accessible error messages.packages/fluent-editor/src/table/modules/table-operation-menu.ts (2)
67-69: Trigger re-initialization with minimal side effects.Calling
this.destroy()uponCHANGE_LANGUAGE_EVENTcan abruptly remove the menu. Consider a more granular approach: reset only the necessary properties and re-render to avoid losing user context, especially if the user is actively interacting with the menu.
75-75: Audit direct usage oflangText.Using
const langText = this.quill.langTextis clear, but ensure consistent usage across the file to prevent confusion betweenlangTextreferences. If large-scale i18n changes arise, consider a single i18n management module to streamline updates.packages/fluent-editor/src/fullscreen/handler.ts (1)
30-32: Improve type safety by avoiding 'any' type assertionThe type assertion to
anyreduces type safety. Consider creating an interface for the toolbar-tip module and using proper typing:+ interface ToolbarTipModule { + hideAllTips(): void; + } - const toolbarTipModule = toolbar.quill.getModule('toolbar-tip') as any; + const toolbarTipModule = toolbar.quill.getModule('toolbar-tip') as ToolbarTipModule;Also applies to: 46-48
packages/fluent-editor/src/assets/toolbar.scss (1)
4-12: Consider the implications of increased selector specificityMoving
.toolbar-tip__tooltipunder thehtmlselector increases specificity, which:
- Makes it harder to override these styles
- Could impact theme customization flexibility
Consider using CSS custom properties (variables) for theming instead of nesting under
htmlto maintain lower specificity while achieving the same encapsulation goals.-html { - .toolbar-tip__tooltip { +.toolbar-tip__tooltip { @include setCssVar(text-color, #ffffff); @include setCssVar(bg-color, #303133); z-index: $fullscreenZIndex-full; color: getCssVar(text-color); background-color: getCssVar(bg-color); - } }
🛑 Comments failed to post (1)
packages/fluent-editor/src/config/types/editor-config.interface.ts (1)
28-31: 💡 Codebase verification
Migration needed: Top-level
langandlangTextproperties are still in useBased on the verification results, there are instances where the old top-level
langandlangTextproperties are still being used:
- In
packages/fluent-editor/src/fluent-editor.ts, the class has top-level propertieslangandlangText- Several demo files still use the top-level configuration:
packages/docs/fluent-editor/demos/toolbar-tip.vue: uses top-levellangpackages/docs/fluent-editor/demos/add-toolbar-item.vue: uses top-levellangpackages/docs/fluent-editor/demos/table-text-options.vue: uses top-levellangTextHowever, the new i18n module implementation is already in use as seen in
packages/docs/fluent-editor/demos/i18n.vue, which properly uses the new structure under the modules configuration.🔗 Analysis chain
Confirm the optional
i18nusage and usage coverage.Wrapping
langandlangTextunderi18nclarifies the intent. Ensure that existing calls referencinglangorlangTextat the top level are properly migrated to thei18nobject.
Let me gather more information to understand the full context of these changes.
Let me try one more search to find any existing i18n object usage in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching any legacy references to top-level lang/langText in config rg -A 5 "lang: " | rg -v "i18n" rg -A 5 "langText: " | rg -v "i18n"Length of output: 2279
Script:
#!/bin/bash # Check the FluentEditor class implementation and its usage of i18n config ast-grep --pattern 'class FluentEditor { $$$ }' # Check for any i18n usage in the codebase rg -A 5 "i18n\."Length of output: 385
Script:
#!/bin/bash # Search for i18n object configuration in the codebase rg -A 5 "i18n: {" # Check the content of the i18n demo file cat packages/docs/fluent-editor/demos/i18n.vueLength of output: 1253
zzxming
added
refactoring
and removed
documentation
github-actions
bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/docs/fluent-editor/demos/header-list.vue (2)
12-13: Add error handling for dynamic import.The dynamic import should include error handling to gracefully handle import failures.
- import('@opentiny/fluent-editor').then(({ default: FluentEditor }) => { + import('@opentiny/fluent-editor') + .then(({ default: FluentEditor }) => { FluentEditor.register({ 'modules/header-list': HeaderList }, true) + }) + .catch(error => { + console.error('Failed to load FluentEditor:', error) + })
Line range hint
69-71: Consider internationalizing the static text.The text "基本用法" should be internationalized for better maintainability and global usage.
Consider using Vue's i18n system:
- <p>基本用法</p> + <p>{{ t('basicUsage') }}</p>And define translations in a separate locale file.
packages/docs/fluent-editor/demos/toolbar-tip.vue (1)
43-61: Add error handling for dynamic importThe dynamic import lacks error handling which could lead to silent failures in production.
onMounted(() => { import('@opentiny/fluent-editor') + .catch((error) => { + console.error('Failed to load FluentEditor:', error) + }) })packages/fluent-editor/src/toolbar/toolbar-tip.ts (3)
7-11: Improve type safety for options parameterThe
Partial<Record<string, any>>type is too permissive. Consider creating a specific interface for the options to improve type safety.-constructor(public quill: FluentEditor, options: Partial<Record<string, any>>) +interface ToolbarTipOptions { + tipTextMap?: Record<string, string | { onShow: (target?: HTMLElement, value?: string) => string }>; + // Add other option properties as needed +} +constructor(public quill: FluentEditor, options: Partial<ToolbarTipOptions>)
20-88: Improve code organization by extracting tooltip map buildersThe
resolveOptionsmethod contains complex logic for building different types of tooltip maps. Consider extracting these into separate methods for better maintainability.+private buildButtonTips(langText: Record<string, string>): Record<string, string> { + return ['bold', 'italic', /* ... */].reduce((map, name) => { + map[name] = langText[name] + return map + }, {} as Record<string, string>) +} +private buildSelectTips(langText: Record<string, string>): Record<string, any> { + return ['color', 'background', /* ... */].reduce((map, name) => { + map[name] = { onShow: () => langText[name] } + return map + }, {}) +} resolveOptions(options: Partial<Record<string, any>>): Record<string, any> { const result = super.resolveOptions(options) if (!this.quill.langText) return result const langText = this.quill.langText - const btnTips = [/* ... */].reduce(/* ... */) - const selectTips = [/* ... */].reduce(/* ... */) + const btnTips = this.buildButtonTips(langText) + const selectTips = this.buildSelectTips(langText) // ... }
89-105: Document tipTextMap override behaviorAdd a comment explaining that user-provided tipTextMap values take precedence over default values.
+ // User-provided tipTextMap values override default tooltip text tipTextMap: { ...textMap, ...options.tipTextMap, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
packages/docs/fluent-editor/.vitepress/sidebar.ts(1 hunks)packages/docs/fluent-editor/demos/add-toolbar-item.vue(1 hunks)packages/docs/fluent-editor/demos/header-list-container.vue(0 hunks)packages/docs/fluent-editor/demos/header-list.vue(1 hunks)packages/docs/fluent-editor/demos/i18n-custom.vue(1 hunks)packages/docs/fluent-editor/demos/i18n.vue(1 hunks)packages/docs/fluent-editor/demos/toolbar-tip.vue(1 hunks)packages/docs/fluent-editor/docs/header-list.md(0 hunks)packages/docs/fluent-editor/docs/other-modules.md(1 hunks)packages/docs/package.json(1 hunks)packages/fluent-editor/package.json(1 hunks)packages/fluent-editor/src/assets/style.scss(0 hunks)packages/fluent-editor/src/assets/toolbar.scss(1 hunks)packages/fluent-editor/src/config/types/editor-config.interface.ts(1 hunks)packages/fluent-editor/src/config/types/type.ts(1 hunks)packages/fluent-editor/src/counter/index.ts(4 hunks)packages/fluent-editor/src/custom-clipboard.ts(2 hunks)packages/fluent-editor/src/fluent-editor.ts(5 hunks)packages/fluent-editor/src/fullscreen/handler.ts(2 hunks)packages/fluent-editor/src/i18n/index.ts(1 hunks)packages/fluent-editor/src/index.ts(1 hunks)packages/fluent-editor/src/link/modules/tooltip.ts(1 hunks)packages/fluent-editor/src/table/formats/header.ts(1 hunks)packages/fluent-editor/src/table/modules/table-operation-menu.ts(2 hunks)packages/fluent-editor/src/toolbar/toolbar-tip.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/fluent-editor/src/assets/style.scss
- packages/docs/fluent-editor/docs/header-list.md
- packages/docs/fluent-editor/demos/header-list-container.vue
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/docs/fluent-editor/demos/i18n-custom.vue
- packages/fluent-editor/src/config/types/type.ts
- packages/docs/package.json
- packages/docs/fluent-editor/docs/other-modules.md
- packages/fluent-editor/src/index.ts
- packages/fluent-editor/src/custom-clipboard.ts
- packages/fluent-editor/src/assets/toolbar.scss
- packages/fluent-editor/src/table/modules/table-operation-menu.ts
- packages/docs/fluent-editor/.vitepress/sidebar.ts
- packages/fluent-editor/src/counter/index.ts
- packages/docs/fluent-editor/demos/add-toolbar-item.vue
- packages/fluent-editor/src/fullscreen/handler.ts
- packages/fluent-editor/package.json
- packages/fluent-editor/src/link/modules/tooltip.ts
- packages/docs/fluent-editor/demos/i18n.vue
- packages/fluent-editor/src/i18n/index.ts
- packages/fluent-editor/src/config/types/editor-config.interface.ts
🔇 Additional comments (8)
packages/docs/fluent-editor/demos/header-list.vue (3)
2-4: LGTM! Proper migration to external package.
The change from internal dependencies to the external quill-header-list package aligns with the PR objectives. The imports are correctly structured.
18-25: Verify toolbar configuration structure.
The toolbar configuration has been significantly changed to an object-based structure. Please ensure this matches the expected format in the documentation.
Consider extracting the toolbar configuration to a separate constant for better maintainability and reusability.
29-29: Configure or remove empty better-table module.
The better-table module is included but has no configuration. Either provide proper configuration or remove if not needed.
Run this script to check if better-table is used elsewhere:
packages/docs/fluent-editor/demos/toolbar-tip.vue (1)
23-41:
Remove header-list from toolbar configuration
The toolbar configuration still includes 'header-list' which contradicts the PR objective of removing header-list dependencies.
['link', 'blockquote', 'code', 'code-block'],
['image', 'file', 'better-table'],
- ['emoji', 'video', 'formula', 'screenshot', 'fullscreen', 'header-list'],
+ ['emoji', 'video', 'formula', 'screenshot', 'fullscreen'],Likely invalid or redundant comment.
packages/fluent-editor/src/table/formats/header.ts (2)
Line range hint 9-24: LGTM: Clean implementation with proper type declarations
The class implementation maintains all necessary functionality while properly declaring types for TypeScript. The static create method correctly delegates to the parent class while preserving custom attribute handling.
1-2: Verify type safety with Quill's header format
The transition from HeaderWithID to OriginHeader looks good and aligns with the PR objective of removing internal dependencies. However, let's ensure type safety:
Also applies to: 6-6, 8-8
✅ Verification successful
Let me gather more information about the header format implementation to ensure type safety.
Type safety is maintained with Quill's header format implementation
The verification shows that:
- The
Headerclass properly extendsOriginHeaderand maintains all necessary type definitions and methods - No other files in the codebase reference
HeaderWithID, confirming clean removal - The implementation preserves all required Quill header functionality including:
- Static methods:
create()andformats() - Instance methods:
format()andoptimize() - Required static properties:
blotNameandtagName
- Static methods:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if HeaderWithID is still referenced elsewhere in the codebase
rg "HeaderWithID"
# Verify if there are any other direct dependencies on quill header format
rg -l "from 'quill/formats/header'"
Length of output: 112
Script:
#!/bin/bash
# Check the implementation details of the header format
ast-grep --pattern 'class $_ extends $_ {
$$$
}' packages/fluent-editor/src/table/formats/header.ts
# Check for any usage or references to header format methods
rg -A 3 "Header\." packages/fluent-editor/src/table/formats/header.ts
Length of output: 9699
packages/fluent-editor/src/fluent-editor.ts (2)
Line range hint 56-146: LGTM: Toolbar handlers properly maintained
The toolbar handlers are well-maintained with proper functionality for formula, table, and list operations. The implementation is clean and maintains backward compatibility.
16-16: Verify i18n integration completeness
The addition of lang and langText properties along with the I18N module import looks good, but let's ensure the integration is complete:
Also applies to: 34-35
✅ Verification successful
I18n integration is properly implemented and complete
The verification shows that the i18n integration is complete and well-implemented:
- I18N module is properly registered in the editor's default modules
- FluentEditor class has the required
langandlangTextproperties - Language change events (CHANGE_LANGUAGE_EVENT) are properly handled across components
- The I18N class has complete implementation with proper language resolution and change management
- Old i18n implementations are properly contained within the new I18N module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining old i18n implementations
rg "resolveLanguageOption|changeLanguage"
# Verify i18n usage across components
rg -l "this\.lang|this\.langText"
Length of output: 792
Script:
#!/bin/bash
# Let's check for i18n module registration and usage
ast-grep --pattern 'class I18N {
$$$
}'
# Check FluentEditor for i18n integration
ast-grep --pattern 'class FluentEditor {
$$$
}'
# Check for i18n-related imports and usage patterns
rg -A 3 "import.*I18N"
Length of output: 3532
Script:
#!/bin/bash
# Let's check for module registration in FluentEditor
rg -A 10 "registerModule|Quill\.register" packages/fluent-editor/src/fluent-editor.ts
# Check for any i18n event handling
rg "CHANGE_LANGUAGE_EVENT|emitter\.emit.*language"
# Check for language-related properties in FluentEditor
ast-grep --pattern 'class FluentEditor extends Quill {
$$$
}'
Length of output: 2492
|
@zzxming 其实我觉得这两个模块demo可以平铺出来，不用移除之前的demo，使用者一般只关注功能，我们的demo只需要把功能演示清楚即可。 后续可以再做一个 API 文档，用来呈现全部的 API 和模块说明等。 |
zzxming
force-pushed
the
chore-module-extract
branch
from
36e3f95 to
91c0888
Compare
保留了之前的 demo，新增一个 demo 用于演示 toolbar-tip 功能 |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
packages/fluent-editor/src/fluent-editor.ts (2)
The review comment is correct -
langandlangTextproperties are not initialized in the constructorBased on the code inspection:
- The
FluentEditorclass declareslang: stringandlangText: Record<string, string>as class properties- The constructor only calls
super(container, options)without initializing these properties- This could indeed lead to undefined behavior when these properties are accessed
The suggested fix in the review comment is appropriate:
constructor(container: HTMLElement | string, options: IEditorConfig = {}) { this.lang = options.lang || 'en' this.langText = options.langText || {} super(container, options) }🔗 Analysis chain
Line range hint
31-34: Verify initialization of new i18n properties.The new
langandlangTextproperties are added but not initialized in the constructor. This could lead to undefined behavior.constructor(container: HTMLElement | string, options: IEditorConfig = {}) { + this.lang = options.lang || 'en' + this.langText = options.langText || {} super(container, options) }
Let me verify the initialization of the new i18n properties by examining the codebase.
Let me search for more specific information about the constructor and interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check the constructor implementation and i18n property usage ast-grep --pattern 'constructor(container: HTMLElement | string, options: IEditorConfig = {}) { $$$ }' # Search for IEditorConfig interface definition ast-grep --pattern 'interface IEditorConfig { $$$ }' # Look for any usage of lang and langText properties rg "lang|langText" packages/fluent-editor/src/fluent-editor.tsLength of output: 278
Script:
#!/bin/bash # Search for the complete FluentEditor class implementation ast-grep --pattern 'export class FluentEditor extends Quill { $$$ }' # Search for the complete IEditorConfig interface rg -A 10 "interface IEditorConfig" packages/fluent-editor/src/fluent-editor.tsLength of output: 862
Dependencies require cleanup in multiple files
The verification shows active dependencies that need to be removed:
Export statements still reference toolbar-tip:
packages/fluent-editor/src/index.ts:export * from './toolbar/toolbar-tip'packages/fluent-editor/src/toolbar/index.ts:export * from './toolbar-tip'Active usage in code:
packages/fluent-editor/src/toolbar/toolbar-tip.ts: Contains 'header-list' reference in codepackages/fluent-editor/src/fullscreen/handler.ts: Usestoolbar-tipmodulepackages/fluent-editor/src/config/i18n/: Both language files contain 'header-list' translationsDocumentation and demo files still present:
- Multiple files in
packages/docs/fluent-editor/containing bothheader-listandtoolbar-tipThese files need to be updated to remove the dependencies completely. The changes should include:
- Removing export statements for toolbar-tip
- Removing the toolbar-tip module usage in fullscreen handler
- Cleaning up i18n translations
- Removing or updating related documentation and demos
🔗 Analysis chain
Line range hint
1-196: Verify complete removal of dependencies.While this file has been updated to remove HeaderList and ToolbarTip dependencies, please ensure all related files mentioned in the previous review are also cleaned up:
- pnpm-lock.yaml
- packages/fluent-editor/src/index.ts
- packages/fluent-editor/src/toolbar/index.ts
- packages/fluent-editor/src/fullscreen/handler.ts
- packages/docs/package.json
- packages/docs/fluent-editor/
- packages/fluent-editor/src/assets/toolbar.scss
- packages/fluent-editor/src/config/i18n/
Dependencies still present in multiple locations
The verification shows that HeaderList and ToolbarTip references are still present in several files:
Package files:
pnpm-lock.yamlSource files:
packages/fluent-editor/src/index.tspackages/fluent-editor/src/toolbar/toolbar-tip.tspackages/fluent-editor/src/toolbar/index.tspackages/fluent-editor/src/fullscreen/handler.tspackages/fluent-editor/src/assets/toolbar.scsspackages/fluent-editor/src/config/i18n/zh-cn.tspackages/fluent-editor/src/config/i18n/en-us.tsDocumentation:
packages/docs/package.jsonpackages/docs/fluent-editor/demos/header-list.vuepackages/docs/fluent-editor/demos/header-list-container.vuepackages/docs/fluent-editor/docs/header-list.mdpackages/docs/fluent-editor/docs/toolbar-tip.mdpackages/docs/fluent-editor/demos/toolbar-tip.vueLet's examine the actual content of these files to understand the extent of the dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify complete removal of dependencies echo "Checking for remaining HeaderList and ToolbarTip references..." # Check package files echo "Checking package files..." rg -l "header-list|toolbar-tip" "package.json" "pnpm-lock.yaml" # Check source files echo "Checking source files..." rg -l "header-list|toolbar-tip" "packages/fluent-editor/src/" # Check documentation echo "Checking documentation..." rg -l "header-list|toolbar-tip" "packages/docs/"Length of output: 1108
Script:
#!/bin/bash # Check the content of source files echo "Content of index.ts:" rg -A 5 "header-list|toolbar-tip" "packages/fluent-editor/src/index.ts" echo -e "\nContent of toolbar files:" rg -A 5 "header-list|toolbar-tip" "packages/fluent-editor/src/toolbar/" echo -e "\nContent of handler.ts:" rg -A 5 "header-list|toolbar-tip" "packages/fluent-editor/src/fullscreen/handler.ts" echo -e "\nContent of i18n files:" rg -A 5 "header-list|toolbar-tip" "packages/fluent-editor/src/config/i18n/"Length of output: 1839
🧹 Nitpick comments (5)
packages/fluent-editor/src/toolbar/toolbar-tip.ts (3)
73-74: Improve type safety for value parameterThe
valueparameter in theonShowcallback could benefit from more specific typing rather than using string.- onShow(target: HTMLElement, value: string) { + onShow(target: HTMLElement, value: 'rtl' | 'ltr' | string) {
77-84: Consider using enum or constants for default valuesThe default values for 'align' and 'header' could be defined as constants to improve maintainability.
+const DEFAULT_VALUES = { + align: 'left', + header: 'normal' +} as const; if (!value) { if (name === 'align') { - value = 'left' + value = DEFAULT_VALUES.align } else if (name === 'header') { - value = 'normal' + value = DEFAULT_VALUES.header } }
90-99: Consider extracting fullscreen tip logicThe fullscreen tip logic could be extracted into a separate function for better readability and reusability.
+ const getFullscreenTip = (quill: FluentEditor, langText: Record<string, string>) => { + return { + onShow: () => langText[quill.isFullscreen ? 'exit-fullscreen' : 'fullscreen'] + } + } const textMap = { ...btnTips, ...valueControlTips, ...selectTips, - fullscreen: { - onShow: () => { - return langText[this.quill.isFullscreen ? 'exit-fullscreen' : 'fullscreen'] - }, - }, + fullscreen: getFullscreenTip(this.quill, langText), }packages/fluent-editor/src/fluent-editor.ts (2)
Line range hint
82-121: Consider refactoring the list handler for better maintainability.The list handler contains complex table-related logic. Consider extracting this into a separate function or utility class to improve readability and maintainability.
- 'list': function (value) { + 'list': function (value) { + this.handleListWithTables(value) + }, + private handleListWithTables(value) { const range = this.quill.getSelection() const formats = this.quill.getFormat(range) // ... rest of the existing logic }
Line range hint
171-171: Remove outdated comment about HeaderList dependency.The comment "make sure register after
HeaderList" is outdated since HeaderList has been removed as an internal dependency.- // make sure register after `HeaderList` 'modules/better-table': BetterTable,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
packages/docs/fluent-editor/.vitepress/sidebar.ts(1 hunks)packages/docs/fluent-editor/demos/add-toolbar-item.vue(1 hunks)packages/docs/fluent-editor/demos/header-list-container.vue(1 hunks)packages/docs/fluent-editor/demos/header-list.vue(1 hunks)packages/docs/fluent-editor/demos/toolbar-tip.vue(1 hunks)packages/docs/fluent-editor/docs/header-list.md(1 hunks)packages/docs/fluent-editor/docs/toolbar-tip.md(1 hunks)packages/docs/package.json(1 hunks)packages/fluent-editor/package.json(1 hunks)packages/fluent-editor/src/assets/style.scss(0 hunks)packages/fluent-editor/src/assets/toolbar.scss(1 hunks)packages/fluent-editor/src/config/types/type.ts(1 hunks)packages/fluent-editor/src/fluent-editor.ts(1 hunks)packages/fluent-editor/src/fullscreen/handler.ts(2 hunks)packages/fluent-editor/src/index.ts(1 hunks)packages/fluent-editor/src/table/formats/header.ts(1 hunks)packages/fluent-editor/src/table/modules/table-operation-menu.ts(1 hunks)packages/fluent-editor/src/toolbar/toolbar-tip.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/fluent-editor/src/assets/style.scss
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/fluent-editor/package.json
- packages/docs/fluent-editor/demos/header-list-container.vue
- packages/fluent-editor/src/fullscreen/handler.ts
- packages/fluent-editor/src/assets/toolbar.scss
- packages/fluent-editor/src/index.ts
- packages/fluent-editor/src/config/types/type.ts
- packages/docs/fluent-editor/docs/header-list.md
- packages/docs/package.json
- packages/docs/fluent-editor/demos/add-toolbar-item.vue
- packages/fluent-editor/src/table/modules/table-operation-menu.ts
- packages/fluent-editor/src/table/formats/header.ts
- packages/docs/fluent-editor/.vitepress/sidebar.ts
- packages/docs/fluent-editor/demos/header-list.vue
🔇 Additional comments (4)
packages/docs/fluent-editor/demos/toolbar-tip.vue (2)
17-19: Avoid assigning libraries to window object
2-2: 🛠️ Refactor suggestion
Use internal module import
Import the QuillToolbarTip through the internal module system to align with PR objectives of managing internal dependencies.
-import QuillToolbarTip from 'quill-toolbar-tip'
+import { QuillToolbarTip } from '@opentiny/fluent-editor'Likely invalid or redundant comment.
packages/fluent-editor/src/fluent-editor.ts (2)
26-26: LGTM: Import changes align with PR objectives.
The removal of HeaderList and ToolbarTip imports while keeping the Toolbar import aligns with the PR objective of removing internal dependencies.
Line range hint 46-54: LGTM: Module configuration maintains compatibility.
The configuration properly includes the i18n module while maintaining compatibility with existing modules like BetterTable.
zzxming
changed the title
zzxming
force-pushed
the
chore-module-extract
branch
from
91c0888 to
dfdee76
Compare
github-actions
bot
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/fluent-editor/src/fluent-editor.ts (1)
Further investigation needed for remaining header-list references
While the
HeaderListclass/component appears to have been removed, there are still several references to "header-list" in the codebase that need attention:
packages/fluent-editor/src/toolbar/toolbar-tip.ts: Contains a toolbar item reference to 'header-list'packages/fluent-editor/src/config/i18n/zh-cn.tsanden-us.ts: Contains translation strings for 'header-list'packages/fluent-editor/src/fluent-editor.ts: Contains a comment referencingHeaderListThese references should be removed as they appear to be remnants of the removed header-list functionality.
🔗 Analysis chain
Line range hint
161-161: Verify removal of header-list dependency.The module registration changes align with the PR objectives. However, let's verify that all header-list references have been properly removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to header-list rg -l "header-list" --type ts --type vueLength of output: 68
Script:
#!/bin/bash # Check for any remaining references to header-list, focusing on TypeScript files rg -l "header-list" --type ts # Also search with different case variations and without hyphen rg -l -i "headerlist|header list" --type ts # Search for specific imports or references in TypeScript files rg "import.*header.*list|from.*header.*list" --type tsLength of output: 322
Script:
#!/bin/bash # Let's check the actual content of these files to understand the context of header-list references echo "=== toolbar-tip.ts ===" rg -C 3 "header-list" packages/fluent-editor/src/toolbar/toolbar-tip.ts echo -e "\n=== zh-cn.ts ===" rg -C 3 "header-list" packages/fluent-editor/src/config/i18n/zh-cn.ts echo -e "\n=== en-us.ts ===" rg -C 3 "header-list" packages/fluent-editor/src/config/i18n/en-us.ts echo -e "\n=== fluent-editor.ts ===" rg -C 3 -i "headerlist|header list" packages/fluent-editor/src/fluent-editor.tsLength of output: 1141
Script:
#!/bin/bash # Let's check for any HeaderList class or type definitions ast-grep --pattern 'class HeaderList' # Also check for any HeaderList imports rg "import.*HeaderList" --type ts # And check for any direct HeaderList usage rg "\bHeaderList\b" --type tsLength of output: 198
♻️ Duplicate comments (1)
packages/docs/fluent-editor/demos/toolbar-tip.vue (1)
17-19:⚠️ Potential issueAvoid assigning libraries to window object.
Assigning libraries directly to the window object is an anti-pattern that can cause global namespace pollution and make testing more difficult.
-window.hljs = hljs -window.katex = katex -window.Html2Canvas = Html2Canvas +// Use these libraries directly where needed
🧹 Nitpick comments (3)
packages/docs/fluent-editor/demos/toolbar-tip.vue (1)
44-59: Add error handling for dynamic import.While the SSR compatibility is handled correctly, consider adding error handling for the dynamic import.
onMounted(() => { import('@opentiny/fluent-editor').then(({ default: FluentEditor, generateToolbarTip }) => { FluentEditor.register({ 'modules/toolbar-tip': generateToolbarTip(QuillToolbarTip) }, true) editor = new FluentEditor(editorRef.value, { // ... configuration }) - }) + }).catch(error => { + console.error('Failed to load FluentEditor:', error) + }) })packages/fluent-editor/src/toolbar/toolbar-tip.ts (1)
7-11: Improve type safety for options parameter.Instead of using
Record<string, any>, consider creating a dedicated interface for the options.+interface ToolbarTipOptions { + tipTextMap?: Record<string, string | { onShow: (target?: HTMLElement, value?: string) => string }>; +} -constructor(public quill: FluentEditor, options: Partial<Record<string, any>>) { +constructor(public quill: FluentEditor, options: Partial<ToolbarTipOptions>) {packages/fluent-editor/src/fluent-editor.ts (1)
Line range hint
33-35: Document new language-related properties.Add JSDoc comments to explain the purpose and expected values of the new language-related properties.
export class FluentEditor extends Quill { isFullscreen: boolean = false options: IEditorConfig & ExpandedQuillOptions + /** Current language code (e.g., 'en', 'zh') */ lang: string + /** Map of language keys to their translated text */ langText: Record<string, string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
packages/docs/fluent-editor/.vitepress/sidebar.ts(1 hunks)packages/docs/fluent-editor/demos/add-toolbar-item.vue(1 hunks)packages/docs/fluent-editor/demos/header-list-container.vue(1 hunks)packages/docs/fluent-editor/demos/header-list.vue(1 hunks)packages/docs/fluent-editor/demos/toolbar-tip.vue(1 hunks)packages/docs/fluent-editor/docs/header-list.md(1 hunks)packages/docs/fluent-editor/docs/toolbar-tip.md(1 hunks)packages/docs/package.json(1 hunks)packages/fluent-editor/package.json(1 hunks)packages/fluent-editor/src/assets/style.scss(0 hunks)packages/fluent-editor/src/assets/toolbar.scss(1 hunks)packages/fluent-editor/src/config/types/type.ts(1 hunks)packages/fluent-editor/src/fluent-editor.ts(1 hunks)packages/fluent-editor/src/fullscreen/handler.ts(2 hunks)packages/fluent-editor/src/index.ts(1 hunks)packages/fluent-editor/src/table/formats/header.ts(1 hunks)packages/fluent-editor/src/table/modules/table-operation-menu.ts(1 hunks)packages/fluent-editor/src/toolbar/toolbar-tip.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/fluent-editor/src/assets/style.scss
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/fluent-editor/package.json
- packages/docs/fluent-editor/.vitepress/sidebar.ts
- packages/fluent-editor/src/config/types/type.ts
- packages/docs/fluent-editor/docs/header-list.md
- packages/docs/fluent-editor/demos/header-list-container.vue
- packages/docs/fluent-editor/docs/toolbar-tip.md
- packages/fluent-editor/src/table/modules/table-operation-menu.ts
- packages/fluent-editor/src/fullscreen/handler.ts
- packages/docs/fluent-editor/demos/add-toolbar-item.vue
- packages/fluent-editor/src/index.ts
- packages/fluent-editor/src/assets/toolbar.scss
- packages/docs/package.json
- packages/docs/fluent-editor/demos/header-list.vue
- packages/fluent-editor/src/table/formats/header.ts
🔇 Additional comments (2)
packages/docs/fluent-editor/demos/toolbar-tip.vue (2)
21-42: LGTM: Well-structured toolbar configuration.
The toolbar configuration is comprehensive and well-organized.
2-2:
Use internal module instead of direct import.
The direct import from 'quill-toolbar-tip' contradicts the PR objective of removing internal dependencies. Use the internal module instead.
-import QuillToolbarTip from 'quill-toolbar-tip'
+import { generateToolbarTip } from '@opentiny/fluent-editor'Likely invalid or redundant comment.
|
@zzxming 目前发现有以下问题：
|
是开发时的问题吧，你要更新一下依赖，之前 docs 里依赖了 Quill，和 fluent-editor 里的 Quill 不是一个版本，有重复依赖而且版本不一致
我提一个 pr 正好把打包错误修复一下 |
|
@zzxming 经过测试发现还有以下问题：
https://github.com/opentiny/fluent-editor/actions/runs/12576375018/job/35052438157 |
新 pr 提交了
这个应该修复了, 这个 action 正常，我本地也能打包 |
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?
Does this PR introduce a breaking change?
Other information
请先合并 #176 以保证此 pr 没有冲突
内部存在两处硬编码(搜索TODO可查看)，之后可再寻找其他解决方案
Summary by CodeRabbit
New Features
Dependencies
quillpackage.quill-header-listandquill-toolbar-tipplugins.Documentation
Refactor