Skip to content

Conversation

dididy
Copy link
Contributor

@dididy dididy commented Sep 1, 2025

What is this PR for?

Unlike the Classic UI, the New UI either lacked implementations for several shortcuts or had shortcuts assigned differently from the Classic UI. I consider this a significant issue, and after addressing other related derivative issues, I am finally submitting it.

d96ae5b - Remove the top margin in the editor search

[AS-IS]
스크린샷 2025-09-01 오후 11 26 14
[TO-BE]
스크린샷 2025-09-01 오후 11 26 23

Fixed an issue where the top area increased by 33px when search was activated. <- referece

0d24eb4 - Handle focus on PARAGRAPH_MOVED receive

When performing "Move Paragraph Up" or "Move Paragraph Down," the paragraph moves up or down after receiving PARAGRAPH_MOVED. During this process, focus was being skipped, so I added it.

1d3d294 - Overriding shortcuts conflict with the editor

In the New UI, we use the Monaco editor, where reserved shortcuts had to be explicitly overridden. I added shortcut functionality for four actions: "Toggle Editor", "Cut the Line", "Paste the Line", and "Search Inside the Code".

57f45e1 - Make action bar searchCode accessible to siblings

There are paragraph.component.ts and action-bar.component.ts that share notebook.component.ts as their parent. In the Classic UI, "Find in Code" is a shortcut that activates the search button in the action-bar. Although this functionality is currently marked as TODO(ZEPPELIN-6279) in the New UI, I set up the connection by adding a Template Reference Variable and Event Binding in notebook.component.html. With this setup, paragraph.component.ts can invoke the searchCode() method of action-bar.component.ts.

b027a33 - Ensure shortcut keys work correctly

image

All shortcut orders have been updated based on the Classic UI. There was logic to handle key bindings differently for macOS, but upon review, it was unnecessary and has been removed. On macOS, typing a combination of Alt and an English letter can produce a different character, so to ensure correct behavior, this resulting character combination is included in the shortcut handling. I added several actions that were not yet implemented.

The distinction between command mode and edit mode for shortcuts was meaningless, so it was removed. The original implementation seemed intended to mimic a vi-like behavior, but this would have made the shortcut system different from the Classic UI.

In the Classic UI, "Auto Completion" had a shortcut to trigger it manually. In the New UI, auto completion is triggered automatically when typing, so "Auto Completion" shortcut was unnecessary and has been removed in this PR.

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

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

Copy link
Contributor

@tbonelee tbonelee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this important task. All of the key bindings you added worked well in my local environment. Perhaps next time we could split this kind of work into separate issues (e.g., fixing just the key bindings themselves and fixing the behavior of each key binding).

@dididy
Copy link
Contributor Author

dididy commented Sep 6, 2025

I corrected the keyboard shortcuts in the dropdown that appears when clicking the settings icon at the top-right of a paragraph; they had differed from the original. The same issue existed in the Classic UI and has been fixed there as well.

e787a62
[NewUI: As-Is]
스크린샷 2025-09-06 오후 3 28 57

[NewUI: To-Be]
스크린샷 2025-09-06 오후 3 28 41

bd4952b
[Classic UI: As-IS]
스크린샷 2025-09-06 오후 3 12 05
[Classic UI: To-Be]
스크린샷 2025-09-06 오후 3 11 50

@tbonelee tbonelee merged commit 65fd70f into apache:master Sep 7, 2025
16 of 18 checks passed
tbonelee pushed a commit that referenced this pull request Sep 7, 2025
…match the Classic UI and work correctly

### What is this PR for?
Unlike the Classic UI, the New UI either lacked implementations for several shortcuts or had shortcuts assigned differently from the Classic UI. I consider this a significant issue, and after addressing other related derivative issues, I am finally submitting it.

#### d96ae5b - Remove the top margin in the editor search
[AS-IS]
<img width="671" height="136" alt="스크린샷 2025-09-01 오후 11 26 14" src="https://github.com/user-attachments/assets/fcd0a107-4b22-42f2-978f-f0781c5fd675" />
[TO-BE]
<img width="669" height="136" alt="스크린샷 2025-09-01 오후 11 26 23" src="https://github.com/user-attachments/assets/475c05e6-ffae-4410-920f-7b0a9e44388d" />

Fixed an issue where the top area increased by 33px when search was activated. <- [referece](https://github.com/microsoft/monaco-editor/blob/5a7ba61be909ae9e4889768a3453ebb0dec392e2/monaco-editor/typedoc/monaco.d.ts#L3473)

#### 0d24eb4 - Handle focus on PARAGRAPH_MOVED receive
When performing "Move Paragraph Up" or "Move Paragraph Down," the paragraph moves up or down after receiving `PARAGRAPH_MOVED`. During this process, focus was being skipped, so I added it.

#### 1d3d294 - Overriding shortcuts conflict with the editor
In the New UI, we use the Monaco editor, where reserved shortcuts had to be explicitly overridden. I added shortcut functionality for four actions: "Toggle Editor", "Cut the Line", "Paste the Line", and "Search Inside the Code".

#### 57f45e1 - Make action bar searchCode accessible to siblings
There are `paragraph.component.ts` and `action-bar.component.ts` that share `notebook.component.ts` as their parent. In the Classic UI, "Find in Code" is a shortcut that activates the search button in the action-bar. Although this functionality is currently marked as [TODO](https://github.com/dididy/zeppelin/blob/fix/ZEPPELIN-6197/6229/zeppelin-web-angular/src/app/pages/workspace/notebook/action-bar/action-bar.component.ts#L220-L222)([ZEPPELIN-6279](https://issues.apache.org/jira/secure/RapidBoard.jspa?rapidView=632&view=detail&selectedIssue=ZEPPELIN-6279)) in the New UI, I set up the connection by adding a Template Reference Variable and Event Binding in `notebook.component.html`. With this setup, `paragraph.component.ts` can invoke the **searchCode()** method of `action-bar.component.ts`.

#### b027a33 - Ensure shortcut keys work correctly
<img width="605" height="1088" alt="image" src="https://github.com/user-attachments/assets/2c4bc264-07e5-4abe-93bb-a2656a86527b" />

All shortcut orders have been updated based on the Classic UI. There was logic to handle key bindings differently for macOS, but upon review, it was unnecessary and has been removed. On macOS, typing a combination of Alt and an English letter can produce a different character, so to ensure correct behavior, this resulting character combination is included in the shortcut handling. I added several actions that were not yet implemented.

The distinction between command mode and edit mode for shortcuts was meaningless, so it was removed. The original implementation seemed intended to mimic a vi-like behavior, but this would have made the shortcut system different from the Classic UI.

In the Classic UI, "Auto Completion" had a shortcut to trigger it manually. In the New UI, auto completion is triggered automatically when typing, so "Auto Completion" shortcut was unnecessary and has been removed in this PR.

### What type of PR is it?
Bug Fix

### Todos

### What is the Jira issue?
* [[ZEPPELIN-6197](https://issues.apache.org/jira/browse/ZEPPELIN-6197)]
* [[ZEPPELIN-6229](https://issues.apache.org/jira/browse/ZEPPELIN-6229)]

### 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

Closes #5064 from dididy/fix/ZEPPELIN-6197/6229.

Signed-off-by: ChanHo Lee <[email protected]>
(cherry picked from commit 65fd70f)
Signed-off-by: ChanHo Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants