Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve undo/redo for Title component #1197

Closed
wants to merge 14 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
margin: 0 auto;
}

mat-form-field {
app-title-editor {
width: 100%;
}

Expand Down
12 changes: 6 additions & 6 deletions src/app/phase-bug-reporting/new-issue/new-issue.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ <h1 class="mat-display-1 title">New Issue</h1>
<div class="form">
<div class="row">
<div class="column left">
<mat-form-field>
<input id="title" formControlName="title" matInput placeholder="Title" required maxlength="256" />
<mat-error *ngIf="title.errors && title.errors['required'] && (title.touched || title.dirty)"> Title required. </mat-error>
<mat-error *ngIf="title.errors && title.errors['maxlength']"> Title cannot exceed 256 characters. </mat-error>
<mat-hint *ngIf="title.value?.length >= 206"> {{ 256 - title.value?.length }} characters remaining. </mat-hint>
</mat-form-field>
<app-title-editor
[id]="'title'"
[titleField]="title"
[titleForm]="this.newIssueForm"
>
</app-title-editor>

<div style="margin: 10px 0 10px 0">
<app-comment-editor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class NewIssueComponent implements OnInit {

ngOnInit() {
this.newIssueForm = this.formBuilder.group({
title: ['', [Validators.required, Validators.maxLength(256)]],
title: [''],
description: [''],
severity: ['', Validators.required],
type: ['', Validators.required]
Expand Down
10 changes: 9 additions & 1 deletion src/app/shared/issue/issue-components.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,20 @@ import { DescriptionComponent } from './description/description.component';
import { DuplicatedIssuesComponent } from './duplicatedIssues/duplicated-issues.component';
import { DuplicateOfComponent } from './duplicateOf/duplicate-of.component';
import { LabelComponent } from './label/label.component';
import { TitleEditorComponent } from './title-editor/title-editor.component';
import { TitleComponent } from './title/title.component';
import { UnsureCheckboxComponent } from './unsure-checkbox/unsure-checkbox.component';

@NgModule({
imports: [SharedModule, CommentEditorModule, MatProgressBarModule, NgxMatSelectSearchModule, MarkdownModule.forChild()],
imports: [
SharedModule,
CommentEditorModule,
MatProgressBarModule,
NgxMatSelectSearchModule,
MarkdownModule.forChild()],
declarations: [
TitleComponent,
TitleEditorComponent,
DescriptionComponent,
LabelComponent,
AssigneeComponent,
Expand All @@ -27,6 +34,7 @@ import { UnsureCheckboxComponent } from './unsure-checkbox/unsure-checkbox.compo
],
exports: [
TitleComponent,
TitleEditorComponent,
DescriptionComponent,
LabelComponent,
AssigneeComponent,
Expand Down
3 changes: 3 additions & 0 deletions src/app/shared/issue/title-editor/title-editor.component.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
mat-form-field {
width: 100%;
}
25 changes: 25 additions & 0 deletions src/app/shared/issue/title-editor/title-editor.component.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<form [formGroup]="titleForm">
<mat-form-field>
<input
(keydown)="onKeyPress($event)"
(beforeinput)="handleBeforeInputChange($event)"
(input)="handleInputChange($event)"
#titleInput
id="{{ this.id }}"
formControlName="{{ this.id }}"
matInput
required
placeholder="{{ this.placeholderText }}"
maxlength="{{ this.maxLength }}"
/>
<mat-error *ngIf="titleField.errors && titleField.errors['required'] && (titleField.touched || titleField.dirty)">
Title required.
</mat-error>
<mat-error *ngIf="titleField.errors && titleField.errors['maxlength']">
Title cannot exceed {{ maxLength }} characters.
</mat-error>
<mat-hint *ngIf="titleField.value?.length >= maxLength - 50">
{{ maxLength - titleField.value?.length }} characters remaining.
</mat-hint>
</mat-form-field>
</form>
134 changes: 134 additions & 0 deletions src/app/shared/issue/title-editor/title-editor.component.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import { Component, ElementRef, Input, OnInit, ViewChild } from '@angular/core';
import { AbstractControl, FormGroup, Validators } from '@angular/forms';
import { UndoRedo } from '../../../core/models/undoredo.model';

const ISSUE_BODY_SIZE_LIMIT = 256;

type textEntry = {
text: string;
selectStart: number;
selectEnd: number;
};

@Component({
selector: 'app-title-editor',
templateUrl: './title-editor.component.html',
styleUrls: ['./title-editor.component.css']
})
export class TitleEditorComponent implements OnInit {

constructor() {}

@Input() titleField: AbstractControl; // Compulsory Input
@Input() titleForm: FormGroup; // Compulsory Input
@Input() id: string; // Compulsory Input

@Input() initialTitle?: string;
placeholderText = 'Title';

@ViewChild('titleInput', { static: true }) titleInput: ElementRef<HTMLInputElement>;

maxLength = ISSUE_BODY_SIZE_LIMIT;

history: UndoRedo<textEntry>;

ngOnInit() {
if (this.initialTitle !== undefined) {
this.titleField.setValue(this.initialTitle);
}

if (this.titleField === undefined || this.titleForm === undefined || this.id === undefined) {
throw new Error("Title Editor's compulsory properties are not defined.");
}

this.titleField.setValidators([Validators.required, Validators.maxLength(this.maxLength)]);
this.history = new UndoRedo<textEntry>(
75,
() => {
return {
text: this.titleInput.nativeElement.value,
selectStart: this.titleInput.nativeElement.selectionStart,
selectEnd: this.titleInput.nativeElement.selectionEnd
};
},
500
);
}

onKeyPress(event: KeyboardEvent) {
if (this.isUndo(event)) {
event.preventDefault();
this.undo();
return;
} else if (this.isRedo(event)) {
this.redo();
event.preventDefault();
return;
}
}

handleBeforeInputChange(event: InputEvent): void {
switch (event.inputType) {
case 'historyUndo':
case 'historyRedo':
// ignore these events that doesn't modify the text
event.preventDefault();
break;
case 'insertFromPaste':
this.history.forceSave(null, true, false);
break;

default:
this.history.updateBeforeChange();
}
}

handleInputChange(event: InputEvent): void {
switch (event.inputType) {
case 'historyUndo':
case 'historyRedo':
// ignore these events that doesn't modify the text
event.preventDefault();
break;
case 'insertFromPaste':
// paste events will be handled exclusively by handleBeforeInputChange
break;

default:
this.history.createDelayedSave();
}
}

private undo(): void {
const entry = this.history.undo();
if (entry === null) {
return;
}
this.titleField.setValue(entry.text);
this.titleInput.nativeElement.setSelectionRange(entry.selectStart, entry.selectEnd);
}

private redo(): void {
const entry = this.history.redo();
if (entry === null) {
return;
}
this.titleInput.nativeElement.value = entry.text;
this.titleInput.nativeElement.setSelectionRange(entry.selectStart, entry.selectEnd);
}

private isUndo(event: KeyboardEvent) {
// prevents undo from firing when ctrl shift z is pressed
if (navigator.platform.indexOf('Mac') === 0) {
return event.metaKey && event.code === 'KeyZ' && !event.shiftKey;
}
return event.ctrlKey && event.code === 'KeyZ' && !event.shiftKey;
}

private isRedo(event: KeyboardEvent) {
if (navigator.platform.indexOf('Mac') === 0) {
return event.metaKey && event.shiftKey && event.code === 'KeyZ';
}
return (event.ctrlKey && event.shiftKey && event.code === 'KeyZ') || (event.ctrlKey && event.code === 'KeyY');
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code is repeated from comment-editor.component.ts. Perhaps these functions can be in undoredo.model.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I think isUndo and isRedo can definitely be moved into undoredo.model.ts, perhaps as a static method. However, I'm not sure if the same should be done for the undo and redo methods, as I personally feel that the handling of values from UndoRedo::undo/UndoRedo::redo should be done in the components using the UndoRedo model, in the interests of keeping UndoRedo more general. Could I perhaps ask for your thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving the undo and redo methods as it is is okay too

5 changes: 5 additions & 0 deletions src/app/shared/issue/title/title.component.css
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,16 @@
.title-button {
display: flex;
flex-direction: row;
justify-content: center;
align-items: center;
margin: 5px;
float: right;
}

app-title-editor {
width: 80%;
}

:host ::ng-deep .mat-progress-spinner {
color: rgba(0, 0, 0, 0.5);
display: inline-block;
Expand Down
48 changes: 25 additions & 23 deletions src/app/shared/issue/title/title.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,28 +20,30 @@ <h1 class="mat-display-1 title">

<div *ngIf="isEditing">
<form [formGroup]="issueTitleForm" #myForm="ngForm" (ngSubmit)="updateTitle(myForm)">
<mat-form-field style="width: 80%">
<input id="title" formControlName="title" matInput placeholder="Title" required maxlength="256" />
<mat-error *ngIf="issueTitleForm.get('title').hasError('required')"> Title is required. </mat-error>
<mat-error *ngIf="issueTitleForm.get('title').hasError('maxlength')"> Title cannot exceed 256 characters. </mat-error>
<mat-hint *ngIf="issueTitleForm.get('title').value?.length >= 206">
{{ 256 - issueTitleForm.get('title').value?.length }} characters remaining.
</mat-hint>
</mat-form-field>

<button type="submit" [disabled]="issueTitleForm.invalid || isSavePending" mat-stroked-button color="primary" class="title-button">
Save
<ng-container #loadingSpinnerContainer></ng-container>
</button>
<button
type="button"
[disabled]="isSavePending"
mat-stroked-button
color="warn"
class="title-button"
(click)="openCancelDialogIfModified()"
>
Cancel
</button>
<div class="row">
<app-title-editor
[id]="'title'"
[titleField]="this.issueTitleForm.get('title')"
[titleForm]="this.issueTitleForm"
>
</app-title-editor>

<div>
<button type="submit" [disabled]="issueTitleForm.invalid" mat-stroked-button color="primary" class="title-button">
Save
<ng-container #loadingSpinnerContainer></ng-container>
</button>
<button
type="button"
[disabled]="isSavePending"
mat-stroked-button
color="warn"
class="title-button"
(click)="openCancelDialogIfModified()"
>
Cancel
</button>
</div>
</div>
</form>
</div>
2 changes: 1 addition & 1 deletion src/app/shared/issue/title/title.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export class TitleComponent implements OnInit {

ngOnInit() {
this.issueTitleForm = this.formBuilder.group({
title: new FormControl('', [Validators.required, Validators.maxLength(256)])
title: ['']
});
// Build the loading service spinner
this.loadingService
Expand Down
76 changes: 76 additions & 0 deletions tests/app/shared/title-editor/title-editor.component.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import { CUSTOM_ELEMENTS_SCHEMA, DebugElement } from '@angular/core';
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { FormControl, FormGroup, FormsModule } from '@angular/forms';
import { By } from '@angular/platform-browser';
import { BrowserAnimationsModule } from '@angular/platform-browser/animations';

import { TitleEditorComponent } from '../../../../src/app/shared/issue/title-editor/title-editor.component';
import { SharedModule } from '../../../../src/app/shared/shared.module';

describe('CommentEditor', () => {
let fixture: ComponentFixture<TitleEditorComponent>;
let debugElement: DebugElement;
let component: TitleEditorComponent;

const TEST_INITIAL_TITLE = 'abc';

beforeEach(() => {
TestBed.configureTestingModule({
declarations: [TitleEditorComponent],
imports: [FormsModule, SharedModule, BrowserAnimationsModule],
schemas: [CUSTOM_ELEMENTS_SCHEMA]
}).compileComponents();

fixture = TestBed.createComponent(TitleEditorComponent);
debugElement = fixture.debugElement;
component = fixture.componentInstance;

// initialize compulsory inputs
const titleField: FormControl = new FormControl('');
const titleForm: FormGroup = new FormGroup({
title: titleField
});
const id = 'title';

// manually inject inputs into the component
component.titleField = titleField;
component.titleForm = titleForm;
component.id = id;
});

describe('text input box', () => {
it('should render', () => {
fixture.detectChanges();

const textBoxDe: DebugElement = debugElement.query(By.css('input'));
expect(textBoxDe).toBeTruthy();
});

it('should contain an empty string if no initial description is provided', () => {
fixture.detectChanges();

const textBox: any = debugElement.query(By.css('input')).nativeElement;
expect(textBox.value).toEqual('');
});

it('should contain an initial description if one is provided', () => {
component.initialTitle = TEST_INITIAL_TITLE;
fixture.detectChanges();

const textBox: any = debugElement.query(By.css('input')).nativeElement;
expect(textBox.value).toEqual(TEST_INITIAL_TITLE);
});

it('should allow users to input text', async () => {
fixture.detectChanges();

const textBox: any = debugElement.query(By.css('input')).nativeElement;
textBox.value = '123';
textBox.dispatchEvent(new Event('input'));

fixture.whenStable().then(() => {
expect(textBox.value).toEqual('123');
});
});
});
chia-yh marked this conversation as resolved.
Show resolved Hide resolved
});