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

Lectures: Disable submit button on invalid form and add tooltip #9846

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<div class="col-12">
<form [formGroup]="form" (ngSubmit)="submitForm()">
<div class="form-group">
<label for="name">{{ 'artemisApp.attachmentUnit.createAttachmentUnit.name' | artemisTranslate }} *</label>
<label for="name">{{ 'artemisApp.attachmentUnit.createAttachmentUnit.name' | artemisTranslate }}*</label>
<input
type="text"
class="form-control"
Expand Down Expand Up @@ -63,7 +63,6 @@
@if (isFileTooBig()) {
<div class="alert alert-danger">
{{ 'artemisApp.attachmentUnit.createAttachmentUnit.fileTooBig' | artemisTranslate }}
{{ 'artemisApp.attachmentUnit.createAttachmentUnit.fileLimitation' | artemisTranslate }}
</div>
}
@if (!fileName() && fileInputTouched) {
Expand Down Expand Up @@ -105,14 +104,16 @@
</div>
<div class="row">
<div class="col-12">
<button id="submitButton" class="btn btn-primary" type="submit" [disabled]="!isFormValid()">
<span jhiTranslate="entity.action.submit"></span>
</button>
@if (hasCancelButton()) {
<button type="button" (click)="cancelForm()" class="btn btn-default">
<fa-icon [icon]="faTimes" />&nbsp;<span jhiTranslate="entity.action.cancel"></span>
<span [ngbTooltip]="tooltipText" container="body" class="d-inline-block">
<button id="submitButton" class="btn btn-primary" type="submit" [disabled]="!isFormValid()">
<span jhiTranslate="entity.action.submit"></span>
</button>
}
@if (hasCancelButton()) {
<button type="button" (click)="cancelForm()" class="btn btn-default">
<fa-icon [icon]="faTimes" />&nbsp;<span jhiTranslate="entity.action.cancel"></span>
</button>
}
</span>
</div>
</div>
</form>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { ACCEPTED_FILE_EXTENSIONS_FILE_BROWSER, ALLOWED_FILE_EXTENSIONS_HUMAN_RE
import { CompetencyLectureUnitLink } from 'app/entities/competency.model';
import { MAX_FILE_SIZE } from 'app/shared/constants/input.constants';
import { toSignal } from '@angular/core/rxjs-interop';
import { TranslateService } from '@ngx-translate/core';

export interface AttachmentUnitFormData {
formProperties: FormProperties;
Expand Down Expand Up @@ -56,6 +57,7 @@ export class AttachmentUnitFormComponent implements OnChanges {
fileName = signal<string | undefined>(undefined);
isFileTooBig = signal<boolean>(false);

private readonly translateService = inject(TranslateService);
private readonly formBuilder = inject(FormBuilder);
form: FormGroup = this.formBuilder.group({
name: [undefined as string | undefined, [Validators.required, Validators.maxLength(255)]],
Expand All @@ -68,7 +70,7 @@ export class AttachmentUnitFormComponent implements OnChanges {
private readonly statusChanges = toSignal(this.form.statusChanges ?? 'INVALID');

isFormValid = computed(() => {
return (this.statusChanges() === 'VALID' || this.fileName()) && !this.isFileTooBig();
return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value !== '' && this.fileName();
});

ngOnChanges(): void {
Expand All @@ -94,6 +96,22 @@ export class AttachmentUnitFormComponent implements OnChanges {
this.isFileTooBig.set(this.file.size > MAX_FILE_SIZE);
}

get tooltipText(): string | null {
// Both name and file are invalid
if (!this.fileInputTouched && this.nameControl?.invalid) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameAndFileRequiredValidationError');
}
// Only file is invalid
if (!this.fileInputTouched) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError');
}
// Only name is invalid
if (this.nameControl?.invalid) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError');
}
return null;
}
Comment on lines +99 to +113
Copy link
Contributor

@florian-glombik florian-glombik Nov 28, 2024

Choose a reason for hiding this comment

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

This approach will have a bad performance as the getter will be updated with each cycle - you can add a console.log in there to see that you will have thousands of logs just by moving your mouse within the view.

While working on another PR (#9655) I am introducing signals for the validation, you might want to stack this PR as you can then re-use the signals and instead of a getter use a computed property which is much more efficient regarding the performance.

(I will link the other PR once I have pushed the branch to remote and created it)
Here is the PR with the signal changes, on which you might want to stack these changes Lectures: Improve and refactor lecture attachment validation #9893

Copy link
Contributor

Choose a reason for hiding this comment

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

You might also want to revise the client guidelines https://docs.artemis.cit.tum.de/dev/guidelines/client#null-and-undefined

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much for the detailed feedback! I really appreciate it.
I'll wait until your PR is merged and then revise the code here.
I will also make sure I follow the guidelines 👍
I guess null and undefined might be mistakes often made by new contributors. Is there perhaps a Coderabbit policy that could be added so that it warns when used?


get nameControl() {
return this.form.get('name');
}
Expand Down
1 change: 1 addition & 0 deletions src/main/webapp/i18n/de/lectureUnit.json
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@
"file": "Datei",
"version": "Version",
"fileRequiredValidationError": "Du musst eine Datei zum Uploaden auswählen.",
"nameAndFileRequiredValidationError": "Der Name ist ein Pflichtfeld. Du musst eine Datei zum Uploaden auswählen.",
"chooseFile": "Datei auswählen",
"fileTooBig": "Die Datei ist zu groß! Die maximale Dateigröße beträgt 20 MB.",
"fileLimitation": "(max. Größe 20 MB)",
Expand Down
1 change: 1 addition & 0 deletions src/main/webapp/i18n/en/lectureUnit.json
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@
"file": "File",
"version": "Version",
"fileRequiredValidationError": "You must select a file to upload.",
"nameAndFileRequiredValidationError": "Name is required. You must select a file to upload.",
"chooseFile": "Choose File",
"fileTooBig": "File is too big! The maximum file size is 20 MB.",
"fileLimitation": "(max. size 20 MB)",
Expand Down
Loading