Add component ch-code-diff-editor and refactoring the ch-code-editor interface
#346
Conversation
dangmarm
added
feature
✅ Deploy Preview for gx-chameleon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ncamera
left a comment
ncamera
left a comment
There was a problem hiding this comment.
Thanks! I leave some comments.
| @Prop() readonly options: CodeDiffEditorOptions = { | ||
| theme: this.theme, | ||
| automaticLayout: true, | ||
| mouseWheelScrollSensitivity: 4, | ||
| mouseWheelZoom: true, | ||
| enableSplitViewResizing: true, | ||
| renderSideBySide: true | ||
| }; |
There was a problem hiding this comment.
I prefer to initialize this parameter in the connectedCallback lifecycle method. With that, we can sort the properties in alphabetical order and avoid issues in the future if the theme property is defined after the options property.
Another solution could be to remove the theme property from the CodeDiffEditorOptions type (using the Omit utility classes from TypeScript) and then extend the options object with the theme in the #setupDiffEditor method.
| @Prop() readonly options: CodeEditorOptions = { | ||
| theme: this.theme, | ||
| automaticLayout: true, | ||
| mouseWheelScrollSensitivity: 4, | ||
| mouseWheelZoom: true, | ||
| tabSize: 2 | ||
| }; |
| componentDidLoad() { | ||
| this.#configureYaml(); | ||
| this.#setupDiffEditor(); | ||
|
|
||
| this.#resizeObserver = new ResizeObserver(entries => { | ||
| const absoluteContentEntry = entries[0].contentRect; | ||
|
|
||
| this.#monacoDiffEditorInstance.layout({ | ||
| width: absoluteContentEntry.width, | ||
| height: absoluteContentEntry.height | ||
| }); | ||
| }); | ||
|
|
||
| this.#resizeObserver.observe(this.#absoluteContentRef); | ||
| } |
There was a problem hiding this comment.
I notice that in the previous implementation we didn't disconnect the observer to avoid memory leaks. Can you add this change in the disconnectedCallback method?
For reference: https://github.com/genexuslabs/chameleon-controls-library/blob/main/src/components/popover/popover.tsx#L1067
| this.#editorId = `ch-editor-${autoId++}`; | ||
| } | ||
|
|
||
| componentDidLoad() { |
There was a problem hiding this comment.
Same comment as before for the memory leak.
| ...(!( | ||
| "originalEditable" in this.options || "readOnly" in this.options | ||
| ) && { | ||
| originalEditable: !this.readonly, | ||
| readOnly: this.readonly | ||
| }) |
There was a problem hiding this comment.
I think is better to use the readonly property as the default value, even if one of the properties (originalEditable or readOnly) is defined. Otherwise, we have to specify the current behavior in the description of the readonly property.
| ...(!( | |
| "originalEditable" in this.options || "readOnly" in this.options | |
| ) && { | |
| originalEditable: !this.readonly, | |
| readOnly: this.readonly | |
| }) | |
| originalEditable: this.options.originalEditable ?? !this.readonly, | |
| readOnly: this.options.readOnly ?? this.readonly |
| this.#monacoDiffEditorInstance.updateOptions({ | ||
| originalEditable: !newReadonly ?? true, | ||
| readOnly: newReadonly ?? false | ||
| }); |
There was a problem hiding this comment.
This implementation should match the logic implemented in the #setupDiffEditor method.
ncamera
changed the title
ch-code-diff-editor and refactoring the ch-code-editor interface
| * If the ´readOnly´ or ´originalEditable´ property is specified in the ´options´ property, | ||
| * this property has no effect. |
There was a problem hiding this comment.
| * If the ´readOnly´ or ´originalEditable´ property is specified in the ´options´ property, | |
| * this property has no effect. | |
| * If the `readOnly` or `originalEditable` property is specified in the `options` property, | |
| * this property has no effect. |
| * - If `mode` === `"editor"` defaults to `false`. | ||
| * - If `mode` === `"diff-editor"` defaults to `true`. | ||
| * Specifies if the editor should be readonly. | ||
| * If the ´readOnly´ property is specified in the ´options´ property, |
There was a problem hiding this comment.
| * If the ´readOnly´ property is specified in the ´options´ property, | |
| * If the `readOnly` property is specified in the `options` property, |
Added ch-code-diff-editor component.
Breaking changes
ch-code-editorcomponent. The diff editor support is now provided by thech-code-diff-editorcomponent.