You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "dididy (via GitHub)" <gi...@apache.org> on 2025/08/30 03:51:38 UTC
[PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
dididy opened a new pull request, #5057:
URL: https://github.com/apache/zeppelin/pull/5057
### What is this PR for?
**Description:**
Cursor behavior in the New UI’s paragraph needs to be fixed for several cursor related actions, including double-clicking, running all above/below, adding(clone), and removing paragraphs.
When **cloneParagraph()** is called, it internally calls **addParagraph()**, which has already been tested. The same addParagraph-related code is also applied in #5044. If either PR is merged first, I will rebase accordingly. I have also confirmed that **cloneParagraph()** works correctly through #5044.
Due to timing issues, I used `setTimeout` for **removeParagraph()** and **doubleClickParagraph()**. Since this is in the UI area, it likely won’t have major side effects, but I will look into it further.
**Expected:**
- When **doubleClickParagraph()** is executed, the cursor should move to the end of the paragraph.
- When **runAllAbove()** or **runAllBelow()** is executed, the current cursor position should be remembered, and after execution, focus should return to the previous cursor position.
- When **addParagraph()** is executed, the newly added paragraph should receive focus.
- When **removeParagraph()** is executed, focus should move to the paragraph that takes the deleted paragraph’s place.
**Actual (New UI):**
- When **doubleClickParagraph()** is executed, the cursor moves to the beginning instead of the end.
- After **runAllAbove()** or **runAllBelow()**, focus is lost completely.
- When **addParagraph()** is executed, the new paragraph does not automatically receive focus.
- After **removeParagraph()**, focus may not move to the correct paragraph.
**[Appropriate action - Classic UI]**
https://github.com/user-attachments/assets/fc0066f7-4e03-4e3b-9d5b-2f33df415ba7
Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph
**[AS-IS]**
https://github.com/user-attachments/assets/f699f788-cf29-4c4c-8c47-2ef34d7962f0
Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph
**[TO-BE]**
https://github.com/user-attachments/assets/1206c524-103f-4328-85ee-04408073b628
Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph
### What type of PR is it?
Bug Fix
### Todos
### What is the Jira issue?
* [[ZEPPELIN-6298](https://issues.apache.org/jira/browse/ZEPPELIN-6298)]
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update? N
* Is there breaking changes for older versions? N
* Does this needs documentation? N
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#discussion_r2322121587
##########
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:
f49113f
After investigating from multiple angles, I found that when receiving `PARAGRAPH_REMOVED` and executing `focusEditor` on the target paragraph, blur events such as `onEditorBlur` and `blurEditor` inside the paragraph component were being triggered, causing an issue.
Unlike `ADD_PARAGRAPH`, which works without special handling, this problem seems to occur because `nzModalService` is involved.
(If `messageService.paragraphRemove` is executed directly inside the paragraph component’s `removeParagraph` function without `nzModalService`, the issue does not occur.)
Since removing `nzModalService` is not an appropriate solution, I introduced an `ignoreBlur` variable to suppress the blur events and re-apply focus as a fix.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#issuecomment-3263456113
I rebased due to conflicts caused by merged PRs.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#issuecomment-3262446232
Thanks for digging into this with multiple approaches.
Do we agree that `focus()` should run only **after** the modal is fully closed?
If so, what do you think about triggering `this.messageService.paragraphRemove(this.paragraph.id)` after the modal closes?
I confirmed that the return value of `nzOnOk()` is emitted to `afterClose` subject. Using that, we can run the paragraph removal (including `focus()` to the appropriate paragraph) once the modal is definitely closed. This worked in my local testing(tested version: https://github.com/tbonelee/zeppelin/commit/7a64b2eb0613543001dd05da6390f8bda7cb9322).
Could you review this approace and share your thoughts?
```diff
- this.nzModalService.confirm({
- nzTitle: 'Delete Paragraph',
- nzContent: 'Do you want to delete this paragraph?',
- nzAutofocus: null,
- nzOnOk: () => {
- this.messageService.paragraphRemove(this.paragraph.id);
- this.cdr.markForCheck();
- }
- });
+ this.nzModalService
+ .confirm({
+ nzTitle: 'Delete Paragraph',
+ nzContent: 'Do you want to delete this paragraph?',
+ nzOnOk: () => true
+ })
+ .afterClose.pipe(takeUntil(this.destroy$))
+ .subscribe(result => {
+ // result is undefined if a user cancels
+ if (result) {
+ this.messageService.paragraphRemove(this.paragraph.id);
+ this.cdr.markForCheck();
+ }
+ });
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#discussion_r2322121587
##########
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:
f49113f
After investigating from multiple angles, I found that when receiving `PARAGRAPH_REMOVED` and executing `focusEditor` on the target paragraph, blur events such as `onEditorBlur` and `blurEditor` inside the paragraph component were being triggered, causing an issue.
Unlike `ADD_PARAGRAPH`, which works without special handling, this problem seems to occur because `nzModalService` is involved.
(If `messageService.paragraphRemove` is executed directly inside the paragraph component’s `removeParagraph` function without `nzModalService`, the issue does not occur)
Since removing `nzModalService` is not an appropriate solution, I introduced an `ignoreBlur` variable to suppress the blur events and re-apply focus as a fix.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#discussion_r2319039205
##########
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:
7ef077a
This code is used to save the current cursor position. The reason it’s needed is that after running **Run all above** or **Run all below**, the cursor position must be remembered when focus returns. Looking back, I realize I didn’t pay close enough attention at the time, and it wasn’t working as intended. So, I revised it in the above commit to use `editor.onDidChangeCursorPosition()` instead.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#discussion_r2319341188
##########
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:
3d48cfe
I didn’t know much about Angular’s lifecycle before, but after looking into `markForCheck()`, I checked it out a bit. In the current situation, it seemed possible to use `this.cdr.detectChanges()`, and it worked as I intended.
`this.cdr.detectChanges()` forces Angular to immediately check and update the current component and its child views, eliminating the need to wait for the next rendering cycle with `setTimeout()`.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee merged PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#discussion_r2319341188
##########
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:
3d48cfe
I didn’t know much about Angular’s lifecycle before, but after looking into `markForCheck()`, I checked it out a bit. In the current situation, it seemed possible to use `this.cdr.detectChanges()`, and it worked as I intended.
`this.cdr.detectChanges()` forces Angular to immediately check and update the current component and its child views, eliminating the need to wait for the next rendering cycle with `setTimeout()`.
However, this doesn’t solve the issue for focusing the Monaco editor(you mentioned below comment). It’s probably due to Monaco’s complex DOM structure and rendering behavior. I’ll need to look into it further.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on code in PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#discussion_r2319235748
##########
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:
I checked, and it works after the change 👍
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#issuecomment-3263757360
Thanks, merged into master and branch-0.12
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#issuecomment-3262393224
```
this.nzModalService
.confirm({
...
})
.afterClose.pipe(takeUntil(this.destroy$))
.subscribe(() => {
// You can execute focus-related events (or other logic) at the point when the modal is closed.
});
```
Other functions that trigger a modal (such as **runAllAbove** and **runAllBelowAndCurrent**) can subscribe to the modal’s close event to execute focus-related logic, and we actually resolved the issue that way before.
However, **removeParagraph** is different: it deletes the current paragraph, and we need to set focus on the paragraph that comes after it. If we handled it the same way as the others, we would end up trying to focus a paragraph that has already been deleted (or is about to be).
The root cause is that Ant Design’s modal steals focus from Monaco. This creates a timing issue: even if we try to focus the next paragraph after deletion, as long as the modal is not fully destroyed, a blur event will still be triggered. I found in the documentation that there’s an [`nzAutofocus`](https://ng.ant.design/version/9.3.x/components/modal/en#api) option, but this only designates which button in the modal should receive initial focus. Since the paragraph we want to focus isn’t inside the modal, this option isn’t useful (I temporarily set it to `null`, but I’ll remove it if we agree it’s unnecessary).
Therefore, the approach I came up with is to wait until the modal is closed and then reapply focus after the last blur event is triggered.
4434380
Previously, I introduced a variable called `ignoreBlur` to work around this. But while looking deeper into the documentation, I discovered that we can check [`nzModalService.openModals.length`](https://ng.ant.design/components/modal/en#related_type_definition) to detect when the modal has closed. So I refactored the code to remove the `ignoreBlur` variable and use this method instead.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#issuecomment-3263301394
I’ve confirmed that your suggested solution works well.
It turns out that instead of invoking `REMOVE_PARAGRAPH` at the moment the **OK** button is clicked,
it should simply be triggered once the modal has closed.
I realize I was approaching it in a more complicated way than necessary.
Thank you very much for taking the time to look into this issue with me.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#issuecomment-3263472656
Could you please check the failing CI job?
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5057:
URL: https://github.com/apache/zeppelin/pull/5057#discussion_r2319036058
##########
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:
0fd4510
No, there wasn’t a specific reason. Since it worked regardless of the order with `markForCheck()`, I didn’t really consider it at first. However, given Angular’s characteristics, I thought it would be more appropriate to place it at the end, so I made the change.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org