Lectures: Disable submit button on invalid form and add tooltip
#9846
Conversation
github-actions
bot
added
the
client
WalkthroughThe changes in this pull request focus on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)📓 Learnings (1)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)🔇 Additional comments (3)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (3)
The import is correctly placed and necessary for internationalization of tooltip messages.
Convert getter to computed signal for better performance The current implementation using a getter might cause performance issues as it will be evaluated on every change detection cycle. As suggested in the previous comments, this should be converted to a computed signal. Consider refactoring to use signals: -get tooltipText(): string | null {
+private tooltipTextSignal = computed(() => {
if (!this.fileInputTouched && this.nameControl?.invalid) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameAndFileRequiredValidationError');
}
if (!this.fileInputTouched) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError');
}
if (this.nameControl?.invalid) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError');
}
return null;
-}
+});
+
+protected get tooltipText(): string | null {
+ return this.tooltipTextSignal();
+}This change will:
Review validation logic changes The current implementation might be too restrictive and contradicts previous feedback. According to the learnings, the OR logic between Let's verify the impact of this change: Consider reverting to the original logic: -return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value !== '' && this.fileName();
+return (this.statusChanges() === 'VALID' || this.fileName()) && !this.isFileTooBig();Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (3)
71-71: Improve type safety and readability of the validation check.While the stricter validation aligns with the PR objectives, the string comparison can be improved.
Consider this more idiomatic approach:
- return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value != '' && this.fileName(); + return this.statusChanges() === 'VALID' && !this.isFileTooBig() && Boolean(this.nameControl?.value?.trim()) && Boolean(this.fileName());This change:
- Uses
trim()to handle whitespace-only values- Uses
Boolean()for explicit type conversion- Maintains null-safety with the optional chaining
Line range hint
82-95: Add file type validation and error handling.The file change handler should validate the file type against the accepted extensions.
Consider adding these validations:
onFileChange(event: Event): void { const input = event.target as HTMLInputElement; if (!input.files?.length) { return; } this.file = input.files[0]; + const fileExtension = this.file.name.split('.').pop()?.toLowerCase(); + if (!fileExtension || !ACCEPTED_FILE_EXTENSIONS_FILE_BROWSER.includes(`.${fileExtension}`)) { + // Reset the input + input.value = ''; + this.file = undefined; + this.fileName.set(undefined); + return; + } this.fileName.set(this.file.name); // automatically set the name in case it is not yet specified if (this.form && (this.nameControl?.value == undefined || this.nameControl?.value == '')) { this.form.patchValue({ // without extension name: this.file.name.replace(/\.[^/.]+$/, ''), }); } this.isFileTooBig.set(this.file.size > MAX_FILE_SIZE); }
Line range hint
38-40: Enhance component with accessibility and cleanup.Consider these improvements for better maintainability and accessibility:
- Implement
OnDestroyto clean up subscriptions:export class AttachmentUnitFormComponent implements OnChanges, OnDestroy { private destroy$ = new Subject<void>(); ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); } }
- Add ARIA attributes to the file input in the template:
<input #fileInput type="file" [attr.aria-label]="'attachment.fileInput' | translate" [attr.aria-invalid]="isFileTooBig()" [attr.aria-describedby]="isFileTooBig() ? 'file-size-error' : undefined" />
- Consider extracting error messages to translation files for better i18n support.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html (1)
108-130: Consider simplifying the tooltip logic.The implementation successfully meets the PR objectives by disabling the submit button and showing appropriate tooltips. However, the tooltip logic could be improved:
- Consider moving the tooltip condition to the component:
get tooltipText(): string | null { if (!this.fileInputTouched && this.nameControl?.invalid) { return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError') + ' ' + this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError'); } if (!this.fileInputTouched) { return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError'); } if (this.nameControl?.invalid) { return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError'); } return null; }
- Consider using
ng-containerinstead ofspanto avoid extra DOM elements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)
📓 Learnings (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)
Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.
🔇 Additional comments (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html (2)
5-5: LGTM! Clean formatting of the required field indicator.
The removal of extra space before the asterisk improves visual consistency while maintaining the required field indication.
111-113: Verify tooltip text concatenation.
The current implementation uses string concatenation with spaces which could be fragile if translations contain trailing/leading spaces.
✅ Verification successful
String concatenation for validation messages is safe
After examining the translation files, the current implementation is safe. The translation strings for both German and English do not contain any leading or trailing spaces:
"nameRequiredValidationError": "Name is required.""fileRequiredValidationError": "You must select a file to upload."
The explicit space concatenation in the template ensures consistent spacing between the messages, making the implementation robust and maintainable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential spacing issues in translations
rg -g '*.json' '"artemisApp\.attachmentUnit\.createAttachmentUnit\.(nameRequiredValidationError|fileRequiredValidationError)"\s*:\s*"[^"]*(\s+$|^\s+)'
Length of output: 152
Script:
#!/bin/bash
# Let's first find the translation files and check the actual content of these specific keys
fd -e json | xargs rg -l "artemisApp.attachmentUnit.createAttachmentUnit"
# Then let's examine the actual translation strings for these specific validation messages
fd -e json | xargs rg "nameRequiredValidationError|fileRequiredValidationError"
Length of output: 2275
laxerhd
changed the title
Lectures: Disable submit button on invalid form and add tooltip to it
|
Could someone please give me some pointers on the tooltip. Should I move the “logic” to the component file as CodeRabbit suggested? Do you think it is necessary to add a new text for the combined tooltip? |
laxerhd
changed the title
Lectures: Disable submit button on invalid form and add tooltip to itLectures: Disable submit button on invalid form and add tooltip
laxerhd
deleted the
bugfix/lectures/disable-submit-button-on-invalid-form-add-tooltip
branch
laxerhd
restored the
bugfix/lectures/disable-submit-button-on-invalid-form-add-tooltip
branch
|
Sorry for the confusion, tried to rename the branch and thus some complications happend... |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (2)
73-73: Enhance form validation robustnessWhile the validation logic is comprehensive, consider these improvements:
- Use strict comparison (
!==) instead of loose comparison (!=) for empty string check- Optimize condition order for better short-circuiting by checking simpler conditions first
-return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value != '' && this.fileName(); +return this.nameControl?.value !== '' && this.fileName() && !this.isFileTooBig() && this.statusChanges() === 'VALID';
Line range hint
87-93: Improve file name handling robustnessConsider these improvements to the file name handling:
- Use strict comparison (
===) for undefined check- Extract the file name extraction logic into a separate method
- Add a comment explaining the auto-naming behavior
-if (this.form && (this.nameControl?.value == undefined || this.nameControl?.value == '')) { +// Auto-set the name from file name if not explicitly provided by user +if (this.form && (this.nameControl?.value === undefined || this.nameControl?.value === '')) { + const fileNameWithoutExtension = this.extractFileNameWithoutExtension(this.file.name); this.form.patchValue({ - // without extension - name: this.file.name.replace(/\.[^/.]+$/, ''), + name: fileNameWithoutExtension, }); } +private extractFileNameWithoutExtension(fileName: string): string { + return fileName.replace(/\.[^/.]+$/, ''); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)
📓 Learnings (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)
Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.
🔇 Additional comments (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (2)
9-9: LGTM: TranslateService import added for internationalization support
The import is correctly placed and follows Angular style guidelines.
72-73: Verify implementation against PR objectives
The changes successfully implement the PR objectives:
- The submit button is properly disabled when no file is uploaded through the enhanced
isFormValidcomputed property - Clear tooltip messages are provided through the new
tooltipTextgetter
Also applies to: 99-114
✅ Verification successful
Let me gather the template file to verify the implementation.
Implementation correctly matches PR objectives
The code inspection confirms that:
- The submit button is properly disabled through
[disabled]="!isFormValid()"binding, which includes file validation - The tooltip is correctly implemented using
[ngbTooltip]="tooltipText"with appropriate validation messages - The form validation logic in
isFormValidcomputed property matches the template's requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the component template uses these properties correctly
rg -l 'attachment-unit-form\.component\.html' | xargs cat
Length of output: 6051
Script:
#!/bin/bash
# Find and show the template file content
fd -e html attachment-unit-form.component.html -x cat {}
Length of output: 6913
.../lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (3)
73-73: Verify the stricter form validation logicThe validation has been enhanced to ensure both name and file are provided before enabling submission. However, the empty string check
nameControl?.value != ''should use strict equality for consistency with TypeScript best practices.- return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value != '' && this.fileName(); + return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value !== '' && this.fileName();
99-113: Consider adding type safety for translation keysThe tooltip implementation looks good with clear conditions and i18n support. However, consider using TypeScript's string literal types to ensure type safety for translation keys.
// Add at the top of the file type TranslationKey = | 'artemisApp.attachmentUnit.createAttachmentUnit.nameAndFileRequiredValidationError' | 'artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError' | 'artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError'; // Then update the getter get tooltipText(): string | null { const translate = (key: TranslationKey) => this.translateService.instant(key); // Rest of the implementation remains the same }
Line range hint
82-92: Improve file handling robustnessThe file handling logic could be improved in several ways:
- Use strict equality checks
- Extract the file extension regex to a constant
- Add null check for form before accessing nameControl
if (!input.files?.length) { return; } this.file = input.files[0]; this.fileName.set(this.file.name); // automatically set the name in case it is not yet specified - if (this.form && (this.nameControl?.value == undefined || this.nameControl?.value == '')) { + const FILE_EXTENSION_REGEX = /\.[^/.]+$/; + if (this.form && this.nameControl && (this.nameControl.value === undefined || this.nameControl.value === '')) { this.form.patchValue({ // without extension - name: this.file.name.replace(/\.[^/.]+$/, ''), + name: this.file.name.replace(FILE_EXTENSION_REGEX, ''), }); }src/main/webapp/i18n/de/lectureUnit.json (1)
176-176: Consider splitting the combined validation messageThe translation follows the informal style guide correctly using "Du" form. However, combining two validation messages ("name required" and "file required") into one key might reduce maintainability and reusability.
Consider splitting into separate keys for better maintainability:
-"nameAndFileRequiredValidationError": "Der Name ist ein Pflichtfeld. Du musst eine Datei zum Uploaden auswählen.", +"nameRequiredValidationError": "Der Name ist ein Pflichtfeld.", +"fileRequiredValidationError": "Du musst eine Datei zum Uploaden auswählen."This approach would:
- Allow reusing individual messages elsewhere
- Make it easier to maintain translations
- Follow the single responsibility principle
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts(4 hunks)src/main/webapp/i18n/de/lectureUnit.json(1 hunks)src/main/webapp/i18n/en/lectureUnit.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)
src/main/webapp/i18n/de/lectureUnit.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
📓 Learnings (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)
Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.
🔇 Additional comments (3)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (2)
9-9: LGTM: TranslateService import added for i18n support
The import is correctly placed and follows Angular style guidelines.
Line range hint 1-166: Verify component structure and memory management
The component follows Angular best practices with:
- Proper use of signals and computed properties
- Clear separation of concerns
- Type safety throughout the component
- Memory leak prevention through proper cleanup
However, consider adding:
- Unit tests for the new validation and tooltip logic
- Error handling for file operations
src/main/webapp/i18n/en/lectureUnit.json (1)
176-176: LGTM! Please verify German translation.
The new validation message effectively combines the name and file requirements, providing clear user feedback. This aligns well with the PR objective to improve the file upload dialog experience.
Let's verify the German translation exists:
✅ Verification successful
German translation is present and accurate ✓
The German translation for the new validation message exists and correctly conveys the same meaning as the English version, combining both the name requirement ("Der Name ist ein Pflichtfeld") and file upload requirement ("Du musst eine Datei zum Uploaden auswählen").
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the German translation for the new validation message exists
rg "nameAndFileRequiredValidationError" "src/main/webapp/i18n/de/lectureUnit.json"
Length of output: 211
HawKhiem
added
deploy:artemis-test6
deploy:artemis-test3
deploy:artemis-test5
and removed
deploy:artemis-test6
deploy:artemis-test3
labels
| 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; | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
You might also want to revise the client guidelines https://docs.artemis.cit.tum.de/dev/guidelines/client#null-and-undefined
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I rebased onto your PR, but I am not sure how I can use it. I have never worked with signals/computed values before, so I would be very happy if I could get some advice on this.
I tried to rewrite the getter, but since it is only calculated when a signal changes, it was not always correct. That's why I added the isFormValid check at the beginning.
tooltipText = computed(() => {
// to trigger the computation
if (this.isFormValid()) {
return undefined;
}
if (!this.fileName() && this.nameControl?.invalid) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameAndFileRequiredValidationError');
}
if (!this.fileName()) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError');
}
if (this.nameControl?.invalid) {
return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError');
}
return undefined;
});
laxerhd
dismissed stale reviews from SimonEntholzer and coderabbitai[bot]
via
5d7b755
laxerhd
force-pushed
the
bugfix/lectures/disable-submit-button-on-invalid-form-add-tooltip
branch
from
d3c7a32
to
5d7b755
Compare
Checklist
General
Server
Client
Motivation and Context
Fixes #9829. The submit button did not indicate that a mandatory field was still missing and could be clicked.
This led to confusion among users.
The error message for files that were too large also had an redundant sentence.
Description
I have updated/changed the conditions in isValidForm to recognize empty string names and uploaded files.
Steps for Testing
Prerequisites:
LectureUnits->FileTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
tooltip-submit-button.mp4
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Improvements