Text-exercises: Highlight selected result in result history timeline
#9845
Conversation
github-actions
bot
added
the
client
EneaGore
changed the title
EneaGore
changed the title
General: Hihglight selected result in result history timeline
EneaGore
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
EneaGore
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
EneaGore
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
github-actions
bot
added
deployment-error
EneaGore
removed
the
deployment-error
FelixTJDietrich
changed the title
General: Hihglight selected result in result history timelineGeneral: Highlight selected result in result history timeline
EneaGore
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/overview/result-history/result-history.component.ts (1)
29-33: Consider memoizing filtered resultsThe filter operations in ngOnChanges are recreated on every change. Consider memoizing the filtered results to optimize performance.
+ private readonly filteredResults = computed(() => { + if (this.exercise()?.type === ExerciseType.TEXT || this.exercise()?.type === ExerciseType.MODELING) { + return this.results().filter((result) => result.successful !== undefined); + } + return this.results(); + }); ngOnChanges(): void { this.showPreviousDivider = this.results().length > MAX_RESULT_HISTORY_LENGTH; - if (this.exercise()?.type === ExerciseType.TEXT || this.exercise()?.type === ExerciseType.MODELING) { - this.displayedResults = this.results().filter((result) => result.successful !== undefined); - } else { - this.displayedResults = this.results(); - } + this.displayedResults = this.filteredResults();src/test/javascript/spec/component/overview/result-history/result-history.component.spec.ts (1)
Line range hint
1-108: Add test coverage for result highlightingThe test suite is missing coverage for the new highlighting functionality introduced by selectedResultId.
Add the following test:
it('should highlight selected result', () => { runInInjectionContext(TestBed, () => { component.results = input<Result[]>([ { rated: true, id: 1 }, { rated: true, id: 2 }, { rated: true, id: 3 }, ]); component.selectedResultId = input<number>(2); }); component.ngOnChanges(); fixture.detectChanges(); const selectedResult = fixture.debugElement.query(By.css('.text-primary')); expect(selectedResult).toBeTruthy(); expect(selectedResult.parent.componentInstance.result.id).toBe(2); });src/main/webapp/app/exercises/text/participate/text-editor.component.html (1)
82-82: Consider extracting the repeated condition into a template variableThe
isExamSummary()function call is repeated in multiple[hidden]bindings. Consider extracting it into a template variable for better performance and maintainability.+@if (submission && !submission.submitted && isExamSummary(); as isHidden) { <span class="badge bg-primary mb-2" id="word-count" - [hidden]="submission && !submission.submitted && isExamSummary()" + [hidden]="isHidden" jhiTranslate="artemisApp.textExercise.wordCount" [translateValues]="{ count: wordCount }"> </span> <span class="badge bg-primary mb-2" id="character-count" - [hidden]="submission && !submission.submitted && isExamSummary()" + [hidden]="isHidden" jhiTranslate="artemisApp.textExercise.characterCount" [translateValues]="{ count: characterCount }"> </span> <textarea id="text-editor" #textEditor class="text-editor-textarea" [maxLength]="MAX_CHARACTER_COUNT" [(ngModel)]="answer" [readOnly]="!isActive || !submission || !isOwnerOfParticipation || isReadOnlyWithShowResult" [disabled]="!isActive || !submission || !isOwnerOfParticipation" (keydown.tab)="onTextEditorTab(textEditor, $event)" (input)="onTextEditorInput($event)" - [hidden]="submission && !submission.submitted && isExamSummary()" + [hidden]="isHidden"> </textarea> +}Also applies to: 90-90, 107-107
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/main/webapp/app/exercises/text/participate/text-editor.component.html(5 hunks)src/main/webapp/app/exercises/text/participate/text-editor.component.ts(6 hunks)src/main/webapp/app/overview/result-history/result-history.component.html(1 hunks)src/main/webapp/app/overview/result-history/result-history.component.ts(2 hunks)src/test/javascript/spec/component/overview/result-history/result-history.component.spec.ts(5 hunks)src/test/javascript/spec/component/text-editor/text-editor.component.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/main/webapp/app/overview/result-history/result-history.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/test/javascript/spec/component/text-editor/text-editor.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/main/webapp/app/exercises/text/participate/text-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/test/javascript/spec/component/overview/result-history/result-history.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/main/webapp/app/overview/result-history/result-history.component.ts (1)
src/main/webapp/app/exercises/text/participate/text-editor.component.ts (1)
🔇 Additional comments (10)
src/main/webapp/app/overview/result-history/result-history.component.ts (1)
20-22: LGTM: Clean migration to signals
The migration from @input decorators to signals using input() follows Angular's latest best practices. The required/optional distinction is properly maintained.
src/main/webapp/app/overview/result-history/result-history.component.html (2)
8-16: LGTM: Clean implementation of result highlighting
The highlighting implementation correctly uses Angular's class binding and follows the PR objective of using primary blue for selected results.
20-20: LGTM: Proper signal usage
The exercise binding correctly uses the function call syntax required for signals.
src/test/javascript/spec/component/overview/result-history/result-history.component.spec.ts (1)
30-36: LGTM: Proper signal initialization in tests
The test setup correctly uses runInInjectionContext for signal initialization, following Angular testing best practices.
src/main/webapp/app/exercises/text/participate/text-editor.component.html (2)
57-57: LGTM: Result history timeline enhancement
The addition of selectedResultId binding aligns with the PR objective to highlight the selected submission in the timeline.
67-67: LGTM: Signal migration for expandProblemStatement
The change from property access to function call is part of the migration to signals for better performance.
src/test/javascript/spec/component/text-editor/text-editor.component.spec.ts (1)
1-1: LGTM: Test updates for signal migration
The test updates correctly use runInInjectionContext to set input properties, following Angular's testing best practices for signal-based inputs.
Also applies to: 113-117
src/main/webapp/app/exercises/text/participate/text-editor.component.ts (3)
61-67: LGTM: Signal-based input declarations
The input properties are correctly migrated to use signals with appropriate default values.
111-111: LGTM: Participation ID handling
The participation ID handling is updated to use the signal-based input while maintaining the fallback to route params.
Also applies to: 123-123
165-165: LGTM: Input value presence checks
The input value checks are correctly updated to use function calls for signal-based inputs.
Also applies to: 176-183
EneaGore
changed the title
General: Highlight selected result in result history timelineText-exercises: Highlight selected result in result history timeline
Checklist
General
- [ ] I tested all changes and their related features with all corresponding user types on a test server.~ [ ] Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.~
Server
No Server Changes
Client
- [ ] I strictly followed the principle of data economy for all client-server REST calls.- [ ] I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.- [ ] I addedauthoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).- [ ] I documented the TypeScript code using JSDoc style.- [ ] I translated all newly inserted strings into English and German.Changes affecting Programming Exercises
No Changes
Motivation and Context
When using the result history timeline on the result view, it is unclear which submission has been selected by the student (apart from the id on the link).
Description
This PR addresses this issue by highlighting the chosen submission. The selected submission is passed on by the text editor component and is checked on the timeline componented whether the highlighted color for the icon is shown. (primary blue to not interfere with green/red/grey)
Also migrates text-editor.component.ts / text-editor.component.ts and result-history.component.ts / result-history.component.ts to using signals
Steps for Testing
Prerequisites:
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
Performance Review
Code Review
Manual Tests
#### Exam Mode Test- [ ] Test 1- [ ] Test 2#### Performance Tests- [ ] Test 1- [ ] Test 2Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests