tbonelee commented on code in PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#discussion_r2316312139
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.ts:
##########
@@ -128,8 +128,15 @@ export class NotebookComponent extends
MessageListenersManager implements OnInit
return;
}
const definedNote = this.note;
- definedNote.paragraphs = definedNote.paragraphs.filter(p => p.id !==
data.id);
- this.cdr.markForCheck();
+ const paragraphIndex = definedNote.paragraphs.findIndex(p => p.id ===
data.id);
+ definedNote.paragraphs.splice(paragraphIndex, 1);
+
+ setTimeout(() => {
+ const adjustedCursorIndex =
+ paragraphIndex === definedNote.paragraphs.length ? paragraphIndex - 1
: paragraphIndex + 1;
+ this.listOfNotebookParagraphComponent.find((_, index) => index ===
adjustedCursorIndex)?.focusEditor();
+ this.cdr.markForCheck();
+ }, 250);
Review Comment:
Is this to ensure that the removed paragraph in `definedNote.paragraphs` is
reflected in `this.listOfNotebookParagraphComponent`?
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.ts:
##########
@@ -142,15 +149,11 @@ export class NotebookComponent extends
MessageListenersManager implements OnInit
return;
}
const definedNote = this.note;
- definedNote.paragraphs.splice(data.index, 0, data.paragraph).map(p => {
- return {
- ...p,
- focus: p.id === data.paragraph.id
- };
- });
- definedNote.paragraphs = [...definedNote.paragraphs];
+ definedNote.paragraphs.splice(data.index, 0, data.paragraph);
+ const paragraphIndex = definedNote.paragraphs.findIndex(p => p.id ===
data.paragraph.id);
+
this.cdr.markForCheck();
- // TODO(hsuanxyz) focus on paragraph
+ definedNote.paragraphs[paragraphIndex].focus = true;
Review Comment:
Is there a specific reason to call `markForCheck()` before setting `.focus`
to `true` instead of after? I'm asking because my understanding of Angular's
change detection rules is still a bit vague.
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts:
##########
@@ -225,6 +253,10 @@ export class NotebookParagraphCodeEditorComponent
implements OnChanges, OnDestro
}
}
if (focus) {
+ if (!focus.currentValue && this.editor) {
+ this.position = this.editor.getPosition();
+ return;
+ }
Review Comment:
What does this code do?
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.ts:
##########
@@ -128,8 +128,15 @@ export class NotebookComponent extends
MessageListenersManager implements OnInit
return;
}
const definedNote = this.note;
- definedNote.paragraphs = definedNote.paragraphs.filter(p => p.id !==
data.id);
- this.cdr.markForCheck();
+ const paragraphIndex = definedNote.paragraphs.findIndex(p => p.id ===
data.id);
+ definedNote.paragraphs.splice(paragraphIndex, 1);
+
+ setTimeout(() => {
+ const adjustedCursorIndex =
+ paragraphIndex === definedNote.paragraphs.length ? paragraphIndex - 1
: paragraphIndex + 1;
+ this.listOfNotebookParagraphComponent.find((_, index) => index ===
adjustedCursorIndex)?.focusEditor();
+ this.cdr.markForCheck();
+ }, 250);
Review Comment:
Was the `setTimeout` value of 250 selected as a heuristic?
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/paragraph.component.ts:
##########
@@ -217,7 +221,8 @@ export class NotebookParagraphComponent extends
ParagraphBase implements OnInit,
if (this.paragraph.config.editorSetting.editOnDblClick &&
this.revisionView !== true) {
this.paragraph.config.editorHide = false;
this.paragraph.config.tableHide = true;
- // TODO(hsuanxyz): focus editor
+ this.focusEditor();
+ setTimeout(() =>
this.notebookParagraphCodeEditorComponent?.setCursorPositionToEnd());
Review Comment:
Is this to ensure that `this.notebookParagraphCodeEditorComponent` is
defined?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
