You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2021/11/03 08:43:54 UTC
[GitHub] [flink] Airblader commented on a change in pull request #17619: [FLINK-23990][runtime-web] Replace custom monaco editor with nz-code-editor
Airblader commented on a change in pull request #17619:
URL: https://github.com/apache/flink/pull/17619#discussion_r741706152
##########
File path: flink-runtime-web/web-dashboard/src/app/pages/job-manager/stdout/job-manager-stdout.component.ts
##########
@@ -28,21 +31,34 @@ import { JobManagerService } from 'services';
styleUrls: ['./job-manager-stdout.component.less'],
changeDetection: ChangeDetectionStrategy.OnPush
})
-export class JobManagerStdoutComponent implements OnInit {
- @ViewChild(MonacoEditorComponent, { static: true }) monacoEditorComponent: MonacoEditorComponent;
+export class JobManagerStdoutComponent implements OnInit, OnDestroy {
stdout = '';
+ loading = true;
+ editorOptions: EditorOptions = flinkEditorOptions;
+ private destroy$ = new Subject<void>();
reload(): void {
- this.jobManagerService.loadStdout().subscribe(data => {
- this.monacoEditorComponent.layout();
- this.stdout = data;
- this.cdr.markForCheck();
- });
+ this.loading = true;
+ this.cdr.markForCheck();
+ this.jobManagerService
+ .loadStdout()
+ .pipe(takeUntil(this.destroy$))
+ .subscribe(data => {
+ // this.monacoEditorComponent.layout();
Review comment:
Unused code
```suggestion
```
##########
File path: flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.html
##########
@@ -16,11 +16,21 @@
~ limitations under the License.
-->
-<nz-tabset [nzSize]="'small'" [nzAnimated]="{ inkBar: true, tabPane: false }">
- <nz-tab nzTitle="Root Exception" nzForceRender>
- <flink-monaco-editor [value]="rootException"></flink-monaco-editor>
+<nz-tabset
+ [nzSize]="'small'"
+ [nzAnimated]="{ inkBar: true, tabPane: false }"
+ [(nzSelectedIndex)]="index"
+>
+ <nz-tab nzTitle="Root Exception">
+ <nz-code-editor
+ *ngIf="index === 0"
+ flinkAutoResize
+ [nzLoading]="isLoading"
+ [ngModel]="rootException"
+ [nzEditorOption]="editorOptions"
+ ></nz-code-editor>
</nz-tab>
- <nz-tab nzTitle="Exception History" nzForceRender>
Review comment:
Just to make sure, I assume `nzForceRender` was used so that the editor would be sized correctly right away, but due to `flinkAutoResize` we no longer need it?
##########
File path: flink-runtime-web/web-dashboard/src/app/pages/job-manager/log-list/job-manager-log-list.component.html
##########
@@ -25,21 +25,25 @@
[nzShowPagination]="false"
[nzVirtualItemSize]="39"
[nzVirtualForTrackBy]="trackByName"
- [nzScroll]="{ y: '700px' }"
+ [nzVirtualMinBufferPx]="480"
+ [nzVirtualMaxBufferPx]="480"
+ [nzScroll]="{ y: '650px' }"
>
<thead>
<tr>
- <th>Log Name</th>
- <th>Size (KB)</th>
+ <th nzWidth="50%">Log Name</th>
+ <th nzWidth="50%">Size (KB)</th>
</tr>
</thead>
<tbody>
- <tr *ngFor="let log of listOfLog">
- <td>
- <a [routerLink]="[log.name]">{{ log.name }}</a>
- </td>
- <td>{{ log.size / 1024 | number: '1.0-2' }}</td>
- </tr>
+ <ng-template nz-virtual-scroll let-data>
Review comment:
`data` will now be untyped whereas before the data was typed properly. This essentially is a limitation in Angular, but I would still like to retain as much type safety as possible here. One way would be to add a utility function
```
export function typeDefinition<T>(): (item: unknown) => T {
return item => item as T;
}
```
Then add something like
```
readonly narrowLogData = typeDefinition<Array<{ name: string; size: number }>>();
```
to the component and use
```
<ng-template nz-virtual-scroll let-untypedData>
<ng-container *ngIf="data as narrowLogData(untypedData)">
…
</ng-container>
</ng-template>
```
This isn't perfect because it's still just a type assertion, but at least it type-checks how we use this type afterwards.
The same applies in other locations.
##########
File path: flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.html
##########
@@ -16,11 +16,21 @@
~ limitations under the License.
-->
-<nz-tabset [nzSize]="'small'" [nzAnimated]="{ inkBar: true, tabPane: false }">
- <nz-tab nzTitle="Root Exception" nzForceRender>
- <flink-monaco-editor [value]="rootException"></flink-monaco-editor>
+<nz-tabset
+ [nzSize]="'small'"
+ [nzAnimated]="{ inkBar: true, tabPane: false }"
+ [(nzSelectedIndex)]="index"
+>
+ <nz-tab nzTitle="Root Exception">
+ <nz-code-editor
+ *ngIf="index === 0"
Review comment:
Why do we need to keep track of the selected tab now?
##########
File path: flink-runtime-web/web-dashboard/src/app/share/common/editor/auto-resize.directive.ts
##########
@@ -0,0 +1,76 @@
+import { Directive, ElementRef, OnDestroy, OnInit, Renderer2 } from '@angular/core';
+import { animationFrameScheduler, interval, Observable, Subject } from 'rxjs';
+import { debounceTime, distinctUntilChanged, takeUntil } from 'rxjs/operators';
+
+import { NzCodeEditorComponent } from 'ng-zorro-antd/code-editor';
+
+@Directive({
+ selector: 'nz-code-editor[flinkAutoResize]'
+})
+export class AutoResizeDirective implements OnDestroy, OnInit {
+ private destroy$ = new Subject();
+ hiddenMinimap = false;
+
+ constructor(
+ private elementRef: ElementRef<HTMLElement>,
+ private nzCodeEditorComponent: NzCodeEditorComponent,
+ private renderer: Renderer2
+ ) {}
+
+ public ngOnInit(): void {
+ this.createResizeObserver()
+ .pipe(
+ distinctUntilChanged((prev, curr) => {
+ const { width: prevWidth, height: prevHeight } = prev;
+ const { width: currWidth, height: currHeight } = curr;
+ return prevWidth === currWidth && prevHeight === currHeight;
+ }),
+ debounceTime(50, animationFrameScheduler),
+ takeUntil(this.destroy$)
+ )
+ .subscribe(curr => {
+ const curWidth = curr.width;
+ this.hiddenMinimap = curWidth <= 65;
Review comment:
Where is this `65` coming from? How was this chosen?
##########
File path: flink-runtime-web/web-dashboard/src/app/share/common/editor/auto-resize.directive.ts
##########
@@ -0,0 +1,76 @@
+import { Directive, ElementRef, OnDestroy, OnInit, Renderer2 } from '@angular/core';
+import { animationFrameScheduler, interval, Observable, Subject } from 'rxjs';
+import { debounceTime, distinctUntilChanged, takeUntil } from 'rxjs/operators';
+
+import { NzCodeEditorComponent } from 'ng-zorro-antd/code-editor';
+
+@Directive({
+ selector: 'nz-code-editor[flinkAutoResize]'
+})
+export class AutoResizeDirective implements OnDestroy, OnInit {
+ private destroy$ = new Subject();
Review comment:
Missing type argument
```suggestion
private destroy$ = new Subject<void>();
```
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org