Remove private members of JS class in BitMarkdownEditor (#10273) #10274
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request refactors the Markdown editor implementation in the BitBlazorUI library. The static Changes
Sequence Diagram(s)sequenceDiagram
participant M as MarkdownEditor
participant E as Editor
participant T as HTMLTextAreaElement
participant D as DotNetObject
M->>E: init(id, textArea, dotnetObj, defaultValue)
E->>T: Setup event listeners in constructor
T->>E: Trigger input events (update selection/caret)
E->>E: Process events and manage content insertion
E->>E: dispose() removes listeners and clears dotnetObj
Assessment against linked issues
Poem
🪧 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
🧹 Nitpick comments (2)
src/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownEditor/BitMarkdownEditor.ts (2)
32-42: Consider making_opensand_pairsprivate.
These fields are currently named with underscores but declared without an access modifier, making them publicly accessible. If they are not intended to be part of the external API, marking them asprivateorprotectedcould reduce public surface area and improve maintainability.-class Editor { - _opens: string[] = []; - _pairs: { [key: string]: string } = { +class Editor { + private _opens: string[] = []; + private _pairs: { [key: string]: string } = {
240-359:keydownmethod is comprehensive but quite large.
While functionally robust, it would benefit from splitting into smaller helper functions for readability (e.g., extracting logic forBackspace,Tab,Enter, andCtrl/Metashortcuts). This can simplify maintenance and testing.async keydown(e: KeyboardEvent) { + if (this.handleBackspace(e)) return; + if (this.handleCtrlShortcuts(e)) return; + ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownEditor/BitMarkdownEditor.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (8)
src/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownEditor/BitMarkdownEditor.ts (8)
4-4: Transition to using theEditorclass looks good.
The updated static_editorsmap, the instantiation ofEditor, and invokingdispose()on teardown align well with the goal of removing private members fromMarkdownEditor. No functional issues spotted here.Also applies to: 7-7, 26-26
54-67: Event listener setup in constructor is solid.
The constructor properly binds and registers event listeners fortextArea. Remember that removing them indispose()is crucial to avoid memory leaks. No issues found in the implementation.
70-75: Getter/setter forvalueare straightforward and clear.
Storing text viathis.textArea.valueis idiomatic and avoids duplication. Good job.
77-88: Validate potential off-by-one scenarios inget block.
The loop incrementstotalbyb.length + 3each iteration to account for '```'. If there’s any mismatch between the code blocks and caret position due to additional spacing or newlines, it might cause inaccurate block calculations. Consider adding tests for edge cases (e.g., caret at the start/end of a block).
119-144: Insertion logic is flexible but watch out for mismatched pairs.
For'wrap'types, the code only pushes to_opensifcontent.value.length < 2, potentially excluding**. If the user is meant to track bold delimiters in_opensfor auto-completion, you may want to handle length ≥ 2 as well. Otherwise, it’s safe to keep as-is if intentional.
228-236: Proper cleanup indispose().
Removing all event listeners and resettingdotnetObjtoundefinedis an appropriate memory management pattern.
362-370: Event handlers fordblClick,click, andchangeare succinct.
They effectively manage selection trimming (dblClick), reset_opensonclick, and notify the .NET object of changes. No concerns here.Also applies to: 373-375, 377-379
382-385: MovedContenttype to the end of the file.
This improves organization by keeping utility types near the bottom and relevant class definitions at the top. This is consistent with typical code style guidelines.
msynk
deleted the
10273-blazorui-markdowneditor-replace-private-js-members
branch
closes #10273
Summary by CodeRabbit