Development: Fix DOM event name conflicts
#9589
Conversation
github-actions
bot
added
client
WalkthroughThe changes in this pull request primarily involve renaming output event emitters across several components to improve naming consistency and clarity. The Changes
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/forms/form-footer/form-footer.component.ts (1)
Line range hint
24-26: Consider enhancing the computed property with i18n and readability improvements.The reactive inputs and computed property implementation follows good practices with Angular signals. However, there are a few suggestions for improvement:
- Ensure the strings are properly translated using the translate service
- Consider making the logic more readable
Here's a suggested improvement for the computed property:
- saveTitle = computed<string>(() => (this.isImport() ? 'entity.action.import' : this.isCreation() ? 'entity.action.generate' : 'entity.action.save')); + saveTitle = computed<string>(() => { + if (this.isImport()) { + return this.translate.instant('entity.action.import'); + } + if (this.isCreation()) { + return this.translate.instant('entity.action.generate'); + } + return this.translate.instant('entity.action.save'); + });Don't forget to:
- Import TranslateService
- Inject it in the constructor
🧰 Tools
🪛 GitHub Check: client-style
[failure] 14-14:
Output bindings, including aliases, should not be named as standard DOM events
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts (1 hunks)
- src/main/webapp/app/forms/form-footer/form-footer.component.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts (1)
src/main/webapp/app/forms/form-footer/form-footer.component.ts (1)
🔇 Additional comments (3)
src/main/webapp/app/forms/form-footer/form-footer.component.ts (1)
13-13: LGTM! Removed linting rule comment improves code consistency.The removal of the linting rule comment aligns with the PR's objective to fix linter issues while maintaining proper Angular event emitter patterns.
src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts (2)
Line range hint
91-95: LGTM! Clean event handling implementation.The dismiss method properly emits the close event and integrates with analytics. The implementation follows Angular best practices for component communication.
Line range hint
17-29: LGTM! Well-structured component following Angular guidelines.The component follows Angular's style guide and best practices:
- Proper use of decorators and access modifiers
- Consistent naming conventions
- Clean implementation of lifecycle hooks
- Appropriate event handling
...p/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/main/webapp/app/forms/form-footer/form-footer.component.ts (1)
Line range hint
34-34: LGTM! Consider improving readabilityThe computed property correctly implements reactive behavior and follows localization guidelines.
Consider improving readability by breaking down the ternary operation:
- saveTitle = computed<string>(() => (this.isImport() ? 'entity.action.import' : this.isCreation() ? 'entity.action.generate' : 'entity.action.save')); + saveTitle = computed<string>(() => { + if (this.isImport()) return 'entity.action.import'; + if (this.isCreation()) return 'entity.action.generate'; + return 'entity.action.save'; + });🧰 Tools
🪛 GitHub Check: client-style
[failure] 15-15:
Output bindings, including aliases, should not be named as standard DOM eventssrc/main/webapp/app/complaints/form/complaints-form.component.ts (2)
Line range hint
47-73: Consider refactoring error handling and validation logicThe
createComplaintmethod has multiple responsibilities and could benefit from some improvements:
- Extract validation logic into a separate method
- Use type-safe error response handling
- Consider using reactive forms for validation instead of direct DOM manipulation
Here's a suggested refactor:
interface ComplaintError { errorKey: string; // add other error properties } private validateComplaint(text: string): boolean { if (text && this.maxComplaintTextLimit < text.length) { this.alertService.error('artemisApp.complaint.exceededComplaintTextLimit', { maxComplaintTextLimit: this.maxComplaintTextLimit! }); return false; } return true; } createComplaint(): void { if (!this.validateComplaint(this.complaintText ?? '')) { return; } const complaintRequest = new ComplaintRequestDTO({ resultId: this.resultId, complaintType: this.complaintType, complaintText: this.complaintText, examId: this.examId }); this.complaintService.create(complaintRequest).subscribe({ next: () => { this.onSubmit.emit(); }, error: (err: HttpErrorResponse<ComplaintError>) => { if (err?.error?.errorKey === 'tooManyComplaints') { this.alertService.error('artemisApp.complaint.tooManyComplaints', { maxComplaintNumber: this.maxComplaintsPerCourse }); } else { onError(this.alertService, err); } }, }); }
Line range hint
77-81: Replace direct DOM manipulation with Angular form controlsThe
complaintTextLengthmethod directly manipulates the DOM, which is not recommended in Angular applications. Consider using reactive forms or template-driven forms instead.Here's a suggested implementation using reactive forms:
import { FormControl } from '@angular/forms'; export class ComplaintsFormComponent implements OnInit { complaintTextControl = new FormControl(''); complaintTextLength(): number { return this.complaintTextControl.value?.length ?? 0; } }Update the template to use the form control:
<textarea [formControl]="complaintTextControl" id="complainTextArea"> </textarea>src/main/webapp/app/exercises/file-upload/assess/file-upload-assessment.component.html (1)
Line range hint
82-115: Consider extracting duplicated template structure.The template structure within
<ng-template #assessment>duplicates the same content from the main template. Consider extracting this shared structure into a separate component or template to improve maintainability and reduce duplication.Example approach:
// Create a new component file: assessment-content.component.ts @Component({ selector: 'jhi-assessment-content', template: ` <jhi-resizeable-container class="col-12"> <!-- Move the shared template structure here --> </jhi-resizeable-container> <div class="row mt-3"> <!-- Move the shared feedback section here --> </div> ` }) export class AssessmentContentComponent { @Input() busy: boolean; @Input() exercise: any; @Input() submission: any; @Input() invalidError: string; @Input() unreferencedFeedback: any[]; @Input() readOnly: boolean; @Input() highlightDifferences: boolean; // ... other required inputs and methods }Then use it in both places:
<!-- In the main template --> @if (submission) { <jhi-assessment-content [busy]="busy" [exercise]="exercise" [submission]="submission" [invalidError]="invalidError" [unreferencedFeedback]="unreferencedFeedback" [readOnly]="readOnly" [highlightDifferences]="highlightDifferences" (feedbacksChange)="validateAssessment()" /> } <!-- In the ng-template --> <ng-template #assessment> <jhi-assessment-content [busy]="busy" [exercise]="exercise" [submission]="submission" [invalidError]="invalidError" [unreferencedFeedback]="unreferencedFeedback" [readOnly]="readOnly" [highlightDifferences]="highlightDifferences" (feedbacksChange)="validateAssessment()" /> </ng-template>src/main/webapp/app/assessment/assessment-header/assessment-header.component.ts (1)
144-148: Consider adding analytics tracking for override submissions.While the event emissions are correctly updated, analytics tracking is only performed for regular submissions but not for override submissions. Consider tracking override submissions separately for better analytics insights.
Apply this diff to add analytics tracking for override submissions:
if (!this.overrideDisabled) { this.onSubmit.emit(); + this.sendSubmitAssessmentEventToAnalytics(true); } else if (!this.submitDisabled) { this.onSubmit.emit(); - this.sendSubmitAssessmentEventToAnalytics(); + this.sendSubmitAssessmentEventToAnalytics(false); }And update the analytics method:
sendSubmitAssessmentEventToAnalytics(isOverride = false) { if (this.exercise?.type === ExerciseType.TEXT) { this.textAssessmentAnalytics.sendAssessmentEvent( isOverride ? TextAssessmentEventType.OVERRIDE_ASSESSMENT : TextAssessmentEventType.SUBMIT_ASSESSMENT ); } }src/test/javascript/spec/component/complaints/complaints-form.component.spec.ts (1)
Line range hint
123-130: Consider breaking down the test for better maintainability.While the test correctly verifies the text limit validation, it could be split into separate test cases:
- Error message validation
- Submit button state validation
- Text limit validation
This would improve test maintainability and make failures easier to diagnose.
Example refactor:
it('should show error message when complaint text exceeds limit', () => { setupComponent(); const errorSpy = jest.spyOn(alertService, 'error'); component.complaintText = 'abcdefghijklmnopqrstuvwxyz'; component.createComplaint(); expect(errorSpy).toHaveBeenCalledExactlyOnceWith( 'artemisApp.complaint.exceededComplaintTextLimit', { maxComplaintTextLimit: 20 } ); }); it('should not emit submit event when complaint text exceeds limit', () => { setupComponent(); const submitSpy = jest.spyOn(component.onSubmit, 'emit'); component.complaintText = 'abcdefghijklmnopqrstuvwxyz'; component.createComplaint(); expect(submitSpy).not.toHaveBeenCalled(); }); function setupComponent() { component.exercise = courseExercise; component.ngOnInit(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
- src/main/webapp/app/assessment/assessment-header/assessment-header.component.html (3 hunks)
- src/main/webapp/app/assessment/assessment-header/assessment-header.component.ts (2 hunks)
- src/main/webapp/app/assessment/assessment-layout/assessment-layout.component.html (1 hunks)
- src/main/webapp/app/assessment/assessment-layout/assessment-layout.component.ts (1 hunks)
- src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.html (1 hunks)
- src/main/webapp/app/complaints/form/complaints-form.component.ts (2 hunks)
- src/main/webapp/app/exercises/file-upload/assess/file-upload-assessment.component.html (1 hunks)
- src/main/webapp/app/exercises/modeling/assess/modeling-assessment-editor/modeling-assessment-editor.component.html (1 hunks)
- src/main/webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.html (1 hunks)
- src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.html (1 hunks)
- src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.html (1 hunks)
- src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts (2 hunks)
- src/main/webapp/app/forms/form-footer/form-footer.component.ts (1 hunks)
- src/test/javascript/spec/component/assessment-shared/assessment-header.component.spec.ts (4 hunks)
- src/test/javascript/spec/component/complaints/complaints-form.component.spec.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts
🧰 Additional context used
📓 Path-based instructions (14)
src/main/webapp/app/assessment/assessment-header/assessment-header.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/assessment/assessment-header/assessment-header.component.ts (1)
src/main/webapp/app/assessment/assessment-layout/assessment-layout.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/assessment/assessment-layout/assessment-layout.component.ts (1)
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.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/complaints/form/complaints-form.component.ts (1)
src/main/webapp/app/exercises/file-upload/assess/file-upload-assessment.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/exercises/modeling/assess/modeling-assessment-editor/modeling-assessment-editor.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/exercises/programming/assess/code-editor-tutor-assessment-container.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/exercises/text/assess/text-submission-assessment.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/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.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/forms/form-footer/form-footer.component.ts (1)
src/test/javascript/spec/component/assessment-shared/assessment-header.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}src/test/javascript/spec/component/complaints/complaints-form.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🪛 GitHub Check: client-style
src/main/webapp/app/forms/form-footer/form-footer.component.ts
[failure] 15-15:
Output bindings, including aliases, should not be named as standard DOM events
🔇 Additional comments (32)
src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.html (2)
Line range hint
16-16: LGTM! Correct usage of new Angular syntax.The template correctly uses the new
@ifsyntax instead of the deprecated*ngIf.
25-25: LGTM! Event naming follows consistent pattern.The event binding has been correctly renamed from
(close)to(onClose)to maintain consistency with other components in the application.src/main/webapp/app/forms/form-footer/form-footer.component.ts (1)
Line range hint
31-32: LGTM! Well-implemented reactive propertiesThe implementation correctly uses Angular's modern reactive primitives with proper typing.
🧰 Tools
🪛 GitHub Check: client-style
[failure] 15-15:
Output bindings, including aliases, should not be named as standard DOM eventssrc/main/webapp/app/assessment/assessment-layout/assessment-layout.component.html (2)
21-22: LGTM! Event binding names follow consistent convention.The renaming of event bindings from
(submit)to(onSubmit)and(cancel)to(onCancel)follows good practices by using the "on" prefix for event handlers, improving code clarity and consistency.
Line range hint
35-45: LGTM! Correct usage of new Angular control flow syntax.The implementation correctly uses the new
@ifsyntax instead of*ngIf, following the provided coding guidelines for Angular templates.src/main/webapp/app/assessment/assessment-layout/assessment-layout.component.ts (1)
Line range hint
1-71: Consider removing unused event emitter.The component still has a
saveevent emitter that coexists withonSubmit. This might lead to confusion about which event to use for saving changes.Let's verify the usage of the
saveemitter:#!/bin/bash # Description: Check if the save emitter is used anywhere in the codebase echo "Checking usage of save emitter..." rg -g '*.html' -A 2 -B 2 '\(save\)'Consider consolidating these events or documenting their distinct purposes if they serve different use cases.
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.html (2)
57-57: LGTM: Event naming follows the new convention.The change from
(submit)to(onSubmit)aligns with the standardization of event names across the application.
Line range hint
1-67: LGTM: Template follows Angular's new control flow syntax.The template correctly uses the new
@ifsyntax instead of the older*ngIf, which aligns with the coding guidelines.src/main/webapp/app/complaints/form/complaints-form.component.ts (2)
22-22: LGTM! Event naming follows Angular style guideThe renaming of the output event from
submittoonSubmitaligns with Angular's style guide recommendations for event naming conventions.
65-65: LGTM! Event emission is consistentThe event emission has been correctly updated to match the renamed output property.
src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.html (2)
21-22: LGTM! Event binding names follow consistent naming pattern.The updated event bindings
(onSubmit)and(onCancel)follow a more consistent and clearer naming pattern. The 'on' prefix better indicates these are event handlers, improving code readability.
Line range hint
29-29: LGTM! Template follows modern Angular syntax.The template correctly uses the new
@ifdirectives instead of the older*ngIfsyntax, following the provided coding guidelines.Also applies to: 52-52, 53-53, 54-54
src/main/webapp/app/exercises/file-upload/assess/file-upload-assessment.component.html (2)
18-19: LGTM: Event binding changes follow consistent naming pattern.The renaming of event bindings from
(submit)to(onSubmit)and(cancel)to(onCancel)aligns with the standardized event naming convention being implemented across components.
Line range hint
24-24: LGTM: Template follows new Angular syntax guidelines.The template correctly uses the new
@ifsyntax instead of the older*ngIfdirectives, adhering to the provided coding guidelines.Also applies to: 27-27, 36-36, 64-64, 69-69, 83-83, 86-86, 104-104
src/main/webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.html (2)
42-43: LGTM! Event emitter naming follows consistent pattern.The renaming of event emitters from
(submit)to(onSubmit)and(cancel)to(onCancel)aligns with the standardized naming convention being applied across components.
Line range hint
1-180: LGTM! Template follows Angular best practices.The template correctly uses the new Angular control flow syntax with
@ifdirectives throughout the file, adhering to the latest Angular standards.src/main/webapp/app/assessment/assessment-header/assessment-header.component.ts (2)
50-51: LGTM! Event emitter naming follows Angular conventions.The renaming of output events to
onSubmitandonCancelimproves clarity and follows Angular's style guide for event naming conventions.
50-51: Verify template bindings for renamed events.Let's ensure all template bindings are updated to use the new event names.
✅ Verification successful
Let me try a different approach to locate the template file since the previous path pattern didn't work.
Template bindings are correctly using the new event names
The template file shows that both
onSubmitandonCancelevent bindings are properly used:
(click)="onSubmit.emit()"is used in two places (regular submit and override buttons)(click)="onCancel.emit()"is used in the cancel button🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if template bindings match the renamed outputs # Test: Search for old and new event names in template echo "Checking template bindings..." rg -l "\\(submit\\)" "src/main/webapp/app/assessment/**/*.html" || echo "No old 'submit' bindings found ✓" rg -l "\\(cancel\\)" "src/main/webapp/app/assessment/**/*.html" || echo "No old 'cancel' bindings found ✓" rg -l "\\(onSubmit\\)" "src/main/webapp/app/assessment/**/*.html" || echo "Warning: No 'onSubmit' bindings found" rg -l "\\(onCancel\\)" "src/main/webapp/app/assessment/**/*.html" || echo "Warning: No 'onCancel' bindings found"Length of output: 996
Script:
#!/bin/bash # First, find the template file echo "Locating template file..." fd -e html assessment-header # If found, check template bindings echo -e "\nChecking template content..." fd -e html assessment-header --exec cat {}Length of output: 10256
src/test/javascript/spec/component/complaints/complaints-form.component.spec.ts (4)
Line range hint
87-91: LGTM! Test follows best practices.The test correctly uses the recommended jest patterns and expectations, with proper spy verification using
toHaveBeenCalledOnce().
Line range hint
96-102: LGTM! Error handling test is well structured.The test properly verifies error scenarios using recommended jest patterns, with correct usage of
not.toHaveBeenCalled()for negative cases.
Line range hint
107-116: LGTM! Known error test is comprehensive.The test thoroughly verifies the error handling path with proper error message validation and follows the recommended testing patterns.
Line range hint
1-174: Verify test coverage for edge cases.While the test suite is comprehensive for the main scenarios, consider adding tests for these edge cases:
- Empty complaint text
- Whitespace-only complaint text
- Special characters in complaint text
- Multiple rapid complaint submissions
src/main/webapp/app/exercises/modeling/assess/modeling-assessment-editor/modeling-assessment-editor.component.html (2)
17-18: LGTM: Event binding changes follow consistent naming pattern.The update from
(submit)/(cancel)to(onSubmit)/(onCancel)aligns with Angular event naming conventions and maintains consistency with other components in the codebase.
Line range hint
1-180: *Consider updating remaining ngIf directives to @if syntax.While most of the template uses the new
@ifsyntax, ensure all conditional logic consistently uses the new Angular control flow syntax as per the coding guidelines.Let's check for any remaining old-style directives:
src/main/webapp/app/assessment/assessment-header/assessment-header.component.html (3)
118-118: LGTM: Event emitter rename follows consistent naming pattern.The change from
submit.emit()toonSubmit.emit()aligns with Angular's event naming conventions while preserving the analytics tracking.
130-130: LGTM: Cancel event emitter rename is consistent.The change from
cancel.emit()toonCancel.emit()maintains consistent event naming patterns while preserving the button's conditional behavior.
Line range hint
1-183: LGTM: Template follows modern Angular syntax.The template correctly uses the new
@ifsyntax throughout, adhering to the coding guidelines that require@ifand@forover*ngIfand*ngFor.src/test/javascript/spec/component/assessment-shared/assessment-header.component.spec.ts (5)
162-164: LGTM: Submit button test updated correctlyThe event emitter rename from
submittoonSubmitis properly reflected in the test, using the correct Jest assertiontoHaveBeenCalledOnce().
167-169: LGTM: Cancel button test updated correctlyThe event emitter rename from
canceltoonCancelis properly reflected in the test, using the correct Jest assertiontoHaveBeenCalledOnce().
192-194: LGTM: Override assessment button test updated correctlyThe event emitter rename from
submittoonSubmitis properly reflected in the test, using the correct Jest assertiontoHaveBeenCalledOnce().
Line range hint
348-352: LGTM: Control+Enter submit test updated correctlyThe event emitter rename from
submittoonSubmitis properly reflected in the test, using the correct Jest assertiontoHaveBeenCalledOnce().
Line range hint
365-369: LGTM: Control+Enter override test updated correctlyThe event emitter rename from
submittoonSubmitis properly reflected in the test, using the correct Jest assertiontoHaveBeenCalledOnce().
src/main/webapp/app/assessment/assessment-layout/assessment-layout.component.ts
Show resolved
Hide resolved
src/main/webapp/app/assessment/assessment-header/assessment-header.component.html
Show resolved
Hide resolved
florian-glombik
changed the title
Development: Fix linter output checkDevelopment: Fix DOM event name conflicts
florian-glombik
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/forms/form-footer/form-footer.component.ts (1)
Line range hint
27-27: Document translation keys for maintainability.The computed property uses what appear to be translation keys. Consider adding a comment documenting the expected translated values for better maintainability.
Apply this diff:
- saveTitle = computed<string>(() => (this.isImport() ? 'entity.action.import' : this.isCreation() ? 'entity.action.generate' : 'entity.action.save')); + /** Translation keys: + * - entity.action.import: "Import" + * - entity.action.generate: "Generate" + * - entity.action.save: "Save" + */ + saveTitle = computed<string>(() => (this.isImport() ? 'entity.action.import' : this.isCreation() ? 'entity.action.generate' : 'entity.action.save'));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise-update.component.html (1 hunks)
- src/main/webapp/app/exercises/modeling/manage/modeling-exercise-update.component.html (1 hunks)
- src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.html (1 hunks)
- src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.html (1 hunks)
- src/main/webapp/app/forms/form-footer/form-footer.component.html (1 hunks)
- src/main/webapp/app/forms/form-footer/form-footer.component.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise-update.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/exercises/modeling/manage/modeling-exercise-update.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/exercises/programming/manage/update/programming-exercise-update.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/exercises/text/manage/text-exercise/text-exercise-update.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/forms/form-footer/form-footer.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/forms/form-footer/form-footer.component.ts (1)
🔇 Additional comments (11)
src/main/webapp/app/forms/form-footer/form-footer.component.ts (1)
Line range hint
24-25: LGTM! Modern Angular input syntax.The reactive properties are well-implemented using Angular's modern input syntax with proper typing.
src/main/webapp/app/forms/form-footer/form-footer.component.html (3)
Line range hint
1-4: LGTM! Proper usage of new Angular control flow syntax.The template correctly uses the new
@ifsyntax instead of*ngIfas per guidelines.
Line range hint
10-14: LGTM! Proper usage of new Angular iteration syntax.The template correctly uses the new
@forsyntax with tracking expression instead of*ngFor.
24-24: LGTM! Proper event handler naming.The change from
cancel.next()toonCancel.next()follows Angular conventions for event handlers and helps avoid conflicts with native DOM events.src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.html (2)
53-53: LGTM! Event binding renamed for consistency.The change from
(cancel)to(onCancel)aligns with Angular's event naming conventions and helps avoid conflicts with native DOM events.
Line range hint
3-7: LGTM! Proper usage of new Angular control flow syntax.The template correctly uses the new
@ifsyntax instead of the older*ngIf, following the latest Angular conventions.src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.html (2)
223-223: LGTM: Event binding rename follows Angular conventions.The change from
(cancel)to(onCancel)correctly avoids conflicts with native DOM events and follows Angular's naming conventions for event handlers.
Line range hint
3-11: LGTM: Template follows new Angular syntax guidelines.The template correctly uses the new
@ifsyntax instead of*ngIfthroughout the file, which aligns with the provided coding guidelines.Also applies to: 16-21, 42-47, 68-72, 82-84
src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise-update.component.html (2)
233-233: LGTM: Event binding renamed to avoid DOM conflicts.The change from
(cancel)to(onCancel)follows best practices by avoiding conflicts with native DOM events and maintains consistency with similar changes across other components.
Line range hint
3-3: LGTM: Consistent usage of new Angular control flow syntax.The template correctly uses the new
@ifsyntax instead of the older*ngIfdirectives throughout the file, following the latest Angular conventions as specified in the coding guidelines.Also applies to: 5-5, 18-18, 63-63, 89-89, 137-137, 142-142, 164-164, 169-169, 174-174, 179-179, 184-184
src/main/webapp/app/exercises/modeling/manage/modeling-exercise-update.component.html (1)
278-278: LGTM! Event binding renamed to avoid DOM conflicts.The change from
(cancel)to(onCancel)follows Angular best practices by prefixing event names with "on" to avoid conflicts with native DOM events.
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.html
Show resolved
Hide resolved
src/main/webapp/app/exercises/modeling/manage/modeling-exercise-update.component.html
Show resolved
Hide resolved
|
github-actions
bot
added
the
deployment-error
florian-glombik
removed
the
deployment-error
HawKhiem
added
deploy:artemis-test2
deploy:artemis-test1
and removed
deploy:artemis-test2
labels
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
I tested on TS1. Canceling assessment, overiding assessment works for all types of exercises. I can also cancel the editing of an exercise as well. However I did find some issues, which might not be directly linked to this PR, but I think should be resolved.
- The overiding of assessment still seems a bit unintuitive for me, it only works if I directly edit my current assessment and that there is only 1 assessment, when I created a few new assessments and click on overide, it always takes the result of the best assessment (the one which gives the hightest points).
Assessment._.testassess._.Practical.Course_.Interactive.Learning.WS24_25.-.Google.Chrome.2024-10-26.17-27-06.mp4
- I tried to assess a Upload exercise where the participant hasn't submitted anything. I presume this behaviour is currently not implemented
Assessment._.Practical.Course_.Interactive.Learning.WS24_25.-.Google.Chrome.2024-10-26.17-19-46.mp4
- I don't know if I did something wrong but somehow I couldn't upload a file in the upload exercise
Submission._.Practical.Course_.Interactive.Learning.WS24_25.-.Google.Chrome.2024-10-26.17-09-31.mp4
- Is it intended that 00000001 is equal to 1 in the assessment section?
Checklist
General
Motivation and Context
When merging develop the linter fails
Re-adding a lintignore that seems to be removed by accident in 5e5e208
The issue appears to be that eslint in some cases simply removes
// eslint-disable-next-line @angular-eslint/no-output-nativeThis does not appear to be the case for
As eslint optimizes away the disable next line and the disable/enable is rather cumbersome to deal with, I think it is a better approach to make sure this issue will not happen again by fixing the underlying naming convention issues.
Description
I renamed the variables to resolve the conflict with the DOM events and conform (for my understanding) with angular conventions.
This is the case for the cancel and submit buttons in the assessment views of the exercises, where name conflicts existed.
Steps for Testing
Testserver 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
Summary by CodeRabbit
New Features
submittoonSubmitandcanceltoonCancel.Bug Fixes
Refactor