Skip to content

Commit 75b6ceb

Browse files
siltomatosiltomato
committed
changes from coderabbit review findings
1 parent 14ecad6 commit 75b6ceb

15 files changed

+122
-93
lines changed

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-editor.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11
import { Injectable } from '@angular/core';
2-
import Quill, { Delta, EmitterSource, Op } from 'quill';
2+
import Quill, { Delta, Op } from 'quill';
33
import { QuillLynxEditorAdapter } from './quill-services/quill-lynx-editor-adapter';
44

55
export type LynxableEditor = Quill; // Add future editor as union type
66

77
export interface LynxEditor {
88
getEditor(): LynxableEditor;
9-
insertText(index: number, text: string, formats?: any): void;
10-
deleteText(index: number, length: number): void;
9+
insertText(index: number, text: string, formats: any, source: any): void;
10+
deleteText(index: number, length: number, source: any): void;
1111
getLength(): number;
12-
formatText(index: number, length: number, formats: any): void;
13-
setContents(delta: any, source: EmitterSource): void;
14-
setSelection(index: number, length: number, source: EmitterSource): void;
12+
formatText(index: number, length: number, formats: any, source: any): void;
13+
setContents(delta: any, source: any): void;
14+
setSelection(index: number, length: number, source: any): void;
1515
getScrollingContainer(): Element;
1616
getBounds(index: number, length: number): any;
17-
updateContents(delta: Delta | Op[]): void;
17+
updateContents(delta: Delta | Op[], source: any): void;
1818
focus(): void;
1919
getRoot(): HTMLElement;
2020
}

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-action-prompt/lynx-insight-action-prompt.component.ts‎

Lines changed: 42 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,19 @@
11
import { Directionality } from '@angular/cdk/bidi';
22
import { Component, DestroyRef, ElementRef, Input, OnInit, Renderer2 } from '@angular/core';
33
import { Bounds } from 'quill';
4-
import { combineLatest, debounceTime, EMPTY, filter, fromEvent, iif, map, startWith, switchMap, tap } from 'rxjs';
4+
import {
5+
combineLatest,
6+
debounceTime,
7+
EMPTY,
8+
filter,
9+
fromEvent,
10+
iif,
11+
map,
12+
Observable,
13+
startWith,
14+
switchMap,
15+
tap
16+
} from 'rxjs';
517
import { quietTakeUntilDestroyed } from 'xforge-common/util/rxjs-util';
618
import { EditorReadyService } from '../base-services/editor-ready.service';
719
import { LynxableEditor, LynxEditor, LynxEditorAdapterFactory } from '../lynx-editor';
@@ -47,8 +59,9 @@ export class LynxInsightActionPromptComponent implements OnInit {
4759
// Adjust prompt vertical position based on line-height
4860
this.yOffsetAdjustment = -this.getLineHeight() / 2;
4961

50-
combineLatest([
51-
this.editorReadyService.listenEditorReadyState(this.lynxEditor.getEditor()).pipe(
62+
const activeInsights$: Observable<LynxInsight[]> = this.editorReadyService
63+
.listenEditorReadyState(this.lynxEditor.getEditor())
64+
.pipe(
5265
filter(loaded => loaded),
5366
switchMap(() => this.editorInsightState.displayState$),
5467
map(displayState =>
@@ -57,28 +70,23 @@ export class LynxInsightActionPromptComponent implements OnInit {
5770
.filter((insight): insight is LynxInsight => insight != null)
5871
),
5972
tap(activeInsights => {
73+
// Keep local copy for click handler
6074
this.activeInsights = activeInsights;
6175
})
62-
),
63-
fromEvent(window, 'resize').pipe(debounceTime(200), startWith(undefined)),
64-
iif(
65-
() => this.lynxEditor?.getScrollingContainer() != null,
66-
fromEvent(this.lynxEditor.getScrollingContainer(), 'scroll').pipe(startWith(undefined)),
67-
EMPTY
68-
)
69-
])
76+
);
77+
78+
const windowResize$ = fromEvent(window, 'resize').pipe(debounceTime(100), startWith(undefined));
79+
80+
const editorScroll$ = iif(
81+
() => this.lynxEditor?.getScrollingContainer() != null,
82+
fromEvent(this.lynxEditor.getScrollingContainer(), 'scroll').pipe(startWith(undefined)),
83+
EMPTY
84+
);
85+
86+
combineLatest([activeInsights$, windowResize$, editorScroll$])
7087
.pipe(quietTakeUntilDestroyed(this.destroyRef))
7188
.subscribe(() => {
72-
const offsetBounds: Bounds | undefined = this.getPromptOffset();
73-
74-
if (offsetBounds != null) {
75-
const boundsEnd: number = this.isLtr ? offsetBounds.right : offsetBounds.left;
76-
this.setHostStyle('top', `${offsetBounds.top + this.yOffsetAdjustment}px`);
77-
this.setHostStyle('left', `${boundsEnd + this.xOffsetAdjustment}px`);
78-
this.setHostStyle('display', 'flex');
79-
} else {
80-
this.setHostStyle('display', 'none');
81-
}
89+
this.updatePosition();
8290
});
8391
}
8492

@@ -94,6 +102,19 @@ export class LynxInsightActionPromptComponent implements OnInit {
94102
this.editorInsightState.toggleDisplayState(['actionOverlayActive']);
95103
}
96104

105+
private updatePosition(): void {
106+
const offsetBounds: Bounds | undefined = this.getPromptOffset();
107+
108+
if (offsetBounds != null) {
109+
const boundsEnd: number = this.isLtr ? offsetBounds.right : offsetBounds.left;
110+
this.setHostStyle('top', `${offsetBounds.top + this.yOffsetAdjustment}px`);
111+
this.setHostStyle('left', `${boundsEnd + this.xOffsetAdjustment}px`);
112+
this.setHostStyle('display', 'flex');
113+
} else {
114+
this.setHostStyle('display', 'none');
115+
}
116+
}
117+
97118
private getPromptOffset(): Bounds | undefined {
98119
if (this.lynxEditor != null) {
99120
const insight: LynxInsight | undefined = getMostNestedInsight(this.activeInsights);

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-editor-objects/lynx-insight-editor-objects.component.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,9 @@ export class LynxInsightEditorObjectsComponent implements OnInit, OnDestroy {
182182
}
183183
}
184184

185+
/**
186+
* Check if two ranges overlap or are adjacent.
187+
*/
185188
function overlaps(x: LynxInsightRange, y: LynxInsightRange): boolean {
186189
return x.index <= y.index + y.length && y.index <= x.index + x.length;
187190
}

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay.service.ts‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,6 @@
1-
import {
2-
CdkScrollable,
3-
Overlay,
4-
OverlayConfig,
5-
OverlayRef,
6-
PositionStrategy,
7-
ScrollDispatcher
8-
} from '@angular/cdk/overlay';
1+
import { CdkScrollable, Overlay, OverlayConfig, OverlayRef, PositionStrategy } from '@angular/cdk/overlay';
92
import { ComponentPortal } from '@angular/cdk/portal';
3+
import { ScrollDispatcher } from '@angular/cdk/scrolling';
104
import { Injectable, NgZone } from '@angular/core';
115
import { asyncScheduler, observeOn, Subject, take, takeUntil } from 'rxjs';
126
import { LynxEditor } from './lynx-editor';
@@ -102,6 +96,11 @@ export class LynxInsightOverlayService {
10296
this.openRef.closed$.next();
10397
this.openRef?.closed$.complete(); // Need null safe operator in case 'closed$.next()' triggers a 'close()' call
10498
this.openRef = undefined;
99+
100+
if (this.scrollableContainer != null) {
101+
this.scrollDispatcher.deregister(this.scrollableContainer);
102+
this.scrollableContainer = undefined;
103+
}
105104
}
106105
}
107106

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ export class LynxInsightOverlayComponent implements OnInit {
114114
return;
115115
}
116116

117-
this.editor.updateContents(action.ops);
117+
this.editor.updateContents(action.ops, 'user');
118118

119119
if (this.focusedInsight == null) {
120120
throw new Error('No focused insight');

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-state.service.spec.ts‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { configureTestingModule } from 'xforge-common/test-utils';
88
import { createTestProjectUserConfig } from '../../../../../../../../RealtimeServer/scriptureforge/models/sf-project-user-config-test-data';
99
import { SFProjectUserConfigDoc } from '../../../../core/models/sf-project-user-config-doc';
1010
import { TextDocId } from '../../../../core/models/text-doc';
11-
import { EDITOR_INSIGHT_DEFAULTS, LynxInsight, LynxInsightConfig } from './lynx-insight';
11+
import { LynxInsight } from './lynx-insight';
1212
import { LynxInsightFilterService } from './lynx-insight-filter.service';
1313
import { LynxInsightStateService } from './lynx-insight-state.service';
1414
import { LynxWorkspaceService } from './lynx-workspace.service';
@@ -37,19 +37,6 @@ describe('LynxInsightStateService', () => {
3737
const mockLynxWorkspaceService = mock<LynxWorkspaceService>();
3838
const mockProjectUserConfigDoc = mock(SFProjectUserConfigDoc);
3939

40-
const defaultConfig: LynxInsightConfig = {
41-
filter: {
42-
types: ['info', 'warning', 'error'],
43-
scope: 'chapter',
44-
includeDismissed: false
45-
},
46-
sortOrder: 'severity',
47-
queryParamName: 'insight',
48-
panelLinkTextMaxLength: 30,
49-
panelLinkTextMinLength: 15,
50-
actionOverlayApplyPrimaryActionChord: { altKey: true, shiftKey: true, key: 'Enter' }
51-
};
52-
5340
let service: LynxInsightStateService;
5441

5542
const rawInsightSource = new Subject<LynxInsight[]>();
@@ -86,8 +73,7 @@ describe('LynxInsightStateService', () => {
8673
{ provide: LynxInsightFilterService, useMock: mockInsightFilterService },
8774
{ provide: ActivatedBookChapterService, useMock: mockActivatedBookChapterService },
8875
{ provide: ActivatedProjectUserConfigService, useMock: mockActivatedProjectUserConfigService },
89-
{ provide: LynxWorkspaceService, useMock: mockLynxWorkspaceService },
90-
{ provide: EDITOR_INSIGHT_DEFAULTS, useValue: defaultConfig }
76+
{ provide: LynxWorkspaceService, useMock: mockLynxWorkspaceService }
9177
]
9278
}));
9379

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-state.service.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ export class LynxInsightStateService {
3838

3939
// Stored filter and order are loaded from project user config
4040
private filterSource$ = new BehaviorSubject<LynxInsightFilter>(this.defaults.filter);
41-
readonly filter$ = this.filterSource$.pipe(distinctUntilChanged());
41+
readonly filter$ = this.filterSource$.pipe(distinctUntilChanged(isEqual), shareReplay(1));
4242
private orderBySource$ = new BehaviorSubject<LynxInsightSortOrder>(this.defaults.sortOrder);
43-
readonly orderBy$ = this.orderBySource$.pipe(distinctUntilChanged());
43+
readonly orderBy$ = this.orderBySource$.pipe(distinctUntilChanged(isEqual), shareReplay(1));
4444

4545
private readonly dismissedInsightIdsSource$ = new BehaviorSubject<string[]>([]);
4646
readonly dismissedInsightIds$ = this.dismissedInsightIdsSource$.pipe(distinctUntilChanged(), shareReplay(1));

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ export interface LynxInsightConfig {
4343
filter: LynxInsightFilter;
4444
sortOrder: LynxInsightSortOrder;
4545
queryParamName: string;
46-
panelLinkTextMaxLength: number;
47-
panelLinkTextMinLength: number;
46+
/** The link text length as an approximate goal. Actual may be slightly smaller or larger due to word boundaries. */
47+
panelLinkTextGoalLength: number;
4848
actionOverlayApplyPrimaryActionChord: Partial<KeyboardEvent>;
4949
}
5050

@@ -63,8 +63,7 @@ export const EDITOR_INSIGHT_DEFAULTS = new InjectionToken<LynxInsightConfig>('ED
6363
filter: { types: ['info', 'warning', 'error'], scope: 'chapter' },
6464
sortOrder: 'severity',
6565
queryParamName: 'insight',
66-
panelLinkTextMaxLength: 30,
67-
panelLinkTextMinLength: 15,
66+
panelLinkTextGoalLength: 30,
6867
actionOverlayApplyPrimaryActionChord: { altKey: true, shiftKey: true, key: 'Enter' }
6968
})
7069
});

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.html‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,13 @@
1212
(click)="onNodeClick(node, $event)"
1313
>
1414
<div class="level-ref">
15-
<li>
15+
<span>
1616
{{ node.name }}
17-
</li>
18-
<mat-icon [matTooltip]="t('restore')" (click)="restoreDismissedInsight(node.insight)" class="restore-icon">
17+
</span>
18+
<!-- TODO: Removed for MVP -->
19+
<!-- <mat-icon [matTooltip]="t('restore')" (click)="restoreDismissedInsight(node.insight)" class="restore-icon">
1920
visibility_on
20-
</mat-icon>
21+
</mat-icon> -->
2122
</div>
2223
</button>
2324
</mat-tree-node>

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.scss‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,17 @@ $restoreActionColor: #5485e7;
7373
display: flex;
7474
align-items: center;
7575
gap: 1rem;
76+
77+
> span:first-child {
78+
display: flex;
79+
align-items: center;
80+
81+
&::before {
82+
content: '•';
83+
margin-right: 0.6em;
84+
font-size: 1.5em;
85+
}
86+
}
7687
}
7788

7889
.level-code {

0 commit comments

Comments
 (0)