General: Show navbar menu in every student view
#8564
Conversation
github-actions
bot
added
the
client
WalkthroughThe recent updates aim to enhance translation support, refine navigation logic, and improve UI consistency across various components. Key changes include adding translation keys, refactoring navigation methods, and updating CSS classes for better styling. Several TypeScript files were modified to remove obsolete subscriptions and streamline route handling. Additionally, new routes and translation entries were added, while some redundant imports and components were removed to optimize the codebase. Changes
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (11)
Files skipped from review as they are similar to previous changes (3)
Additional Context UsedBiome (31)
Path-based Instructions (3)
Additional comments not posted (8)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
rabeatwork
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
rabeatwork
force-pushed
the
chore/show-navbar-in-every-student-view
branch
from
3a840e5
to
e96ecef
Compare
rabeatwork
force-pushed
the
chore/show-navbar-in-every-student-view
branch
from
bd1d20d
to
e052cb7
Compare
src/main/webapp/app/overview/course-lectures/course-lectures.component.html
Show resolved
Hide resolved
src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card/sidebar-card.component.ts
Outdated
Show resolved
Hide resolved
|
github-actions
bot
added
the
deployment-error
rabeatwork
removed
the
deployment-error
rabeatwork
force-pushed
the
chore/show-navbar-in-every-student-view
branch
2 times, most recently
from
c9ecee1
to
6edc760
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range and nitpick comments (38)
src/test/javascript/spec/component/shared/sidebar/sidebar-card-small.component.spec.ts (2)
Line range hint
24-34: Avoid duplicate setup and teardown hooks.Consider consolidating the repeated setup logic in
beforeEachinto a single block to improve code maintainability and reduce redundancy.
Line range hint
1-1: Optimize imports.Some imports are only used as types and should be imported using
import type { Type } from 'module';to clarify their usage and potentially improve build optimization.src/test/javascript/spec/component/shared/sidebar/sidebar-card-large.component.spec.ts (2)
Line range hint
24-34: Avoid duplicate setup and teardown hooks.Consider consolidating the repeated setup logic in
beforeEachinto a single block to improve code maintainability and reduce redundancy.
Line range hint
1-1: Optimize imports.Some imports are only used as types and should be imported using
import type { Type } from 'module';to clarify their usage and potentially improve build optimization.src/main/webapp/app/app.module.ts (1)
Line range hint
26-27: Optimize imports.Some imports are only used as types and should be imported using
import type { Type } from 'module';to clarify their usage and potentially improve build optimization.src/main/webapp/app/overview/course-competencies/course-competencies.component.ts (2)
Line range hint
68-68: Use strict equality for comparisons.Always use strict equality (
===) for comparisons to avoid unexpected type coercion.- return competency.userProgress.first()!.progress == 100 && competency.userProgress.first()!.confidence! >= competency.masteryThreshold!; + return competency.userProgress.first()!.progress === 100 && competency.userProgress.first()!.confidence! >= competency.masteryThreshold!;
Line range hint
85-86: Avoid non-null assertions.Using non-null assertions (
!) can lead to runtime errors if the assumptions about data presence are incorrect. Consider adding checks or using optional chaining.src/test/javascript/spec/component/shared/sidebar/sidebar-card-medium.component.spec.ts (2)
Line range hint
25-35: Avoid duplicate setup and teardown hooks.Consider consolidating the repeated setup logic in
beforeEachinto a single block to improve code maintainability and reduce redundancy.
Line range hint
1-1: Optimize imports.Some imports are only used as types and should be imported using
import type { Type } from 'module';to clarify their usage and potentially improve build optimization.src/test/javascript/spec/component/course/header-course.component.spec.ts (4)
Line range hint
40-40: Simplify the computed expression.Consider using a direct assignment or a simpler method to achieve the same result without concatenation.
Line range hint
49-49: Use template literals for string concatenation.Replace string concatenation with template literals for better readability and performance:
- 'a'.repeat(564) + '…' + `a`.repeat(564) + '…'Also applies to: 59-60, 73-75
Line range hint
70-70: Simplify the computed expression.Consider using a direct assignment or a simpler method to achieve the same result without concatenation.
Line range hint
1-1: Optimize import statements.Some imports are only used as types and should be imported using
import type. This can help with tree shaking and potentially reduce bundle sizes:- import { ComponentFixture, TestBed } from '@angular/core/testing'; + import type { ComponentFixture, TestBed } from '@angular/core/testing';Also applies to: 3-4
src/main/webapp/app/overview/course-exercises/course-exercises.component.ts (2)
Line range hint
34-34: Remove unnecessary type annotations.Type annotations on properties initialized with values are inferred by TypeScript and can be omitted for cleaner code:
- sidebarData: SidebarData; - sidebarExercises: SidebarCardElement[] = []; + sidebarData; + sidebarExercises = [];Also applies to: 38-38
Line range hint
90-90: Use template literals for string concatenation.Replace string concatenation with template literals for better readability and performance:
- 'sidebar.lastSelectedItem.exercise.byCourse.' + this.courseId + `sidebar.lastSelectedItem.exercise.byCourse.${this.courseId}`src/main/webapp/app/overview/exercise-details/course-exercise-details.module.ts (1)
Line range hint
2-3: Optimize import statements.Some imports are only used as types and should be imported using
import type. This can help with tree shaking and potentially reduce bundle sizes:- import { RouterModule, Routes } from '@angular/router'; + import type { RouterModule, Routes } from '@angular/router';src/main/webapp/app/app-routing.module.ts (1)
Line range hint
1-2: Optimize import statements.Some imports are only used as types and should be imported using
import type. This can help with tree shaking and potentially reduce bundle sizes:- import { NgModule } from '@angular/core'; - import { RouterModule, Routes } from '@angular/router'; + import type { NgModule } from '@angular/core'; + import type { RouterModule, Routes } from '@angular/router';Also applies to: 4-5
src/main/webapp/app/overview/course-competencies/course-competencies-details.component.ts (2)
Line range hint
58-58: Avoid using non-null assertions.Using non-null assertions can lead to runtime errors if the values are actually null. Consider checking for null or undefined before usage or using optional chaining.
Also applies to: 60-60, 61-61, 88-88, 119-119, 123-123, 125-125
Line range hint
81-81: Avoid assignments within expressions.Assignments within expressions can lead to less readable and potentially error-prone code. Consider separating the assignment from the expression:
- setTimeout(() => (this.showFireworks = true), 1000); + setTimeout(() => { + this.showFireworks = true; + }, 1000);Also applies to: 82-82
src/test/javascript/spec/component/competencies/course-competencies.component.spec.ts (2)
Line range hint
22-22: Specify explicit types instead ofany.Using
anycan lead to less type-safe code. Specify explicit types for better type checking and code clarity:- parent: any; - params: any; + parent: ActivatedRoute | null; + params: Observable<Params>;Also applies to: 23-23, 25-25
Line range hint
1-1: Optimize import statements.Some imports are only used as types and should be imported using
import type. This can help with tree shaking and potentially reduce bundle sizes:- import { ComponentFixture, TestBed } from '@angular/core/testing'; - import { Competency, CompetencyProgress } from 'app/entities/competency.model'; + import type { ComponentFixture, TestBed } from '@angular/core/testing'; + import type { Competency, CompetencyProgress } from 'app/entities/competency.model';Also applies to: 5-6
src/test/javascript/spec/component/course/course-overview.component.spec.ts (6)
Line range hint
116-116: Specify explicit types instead of usingany.- const course = { - id: 1, - courseInformationSharingConfiguration: CourseInformationSharingConfiguration.COMMUNICATION_AND_MESSAGING, - } as Course; + const course: Course = { + id: 1, + courseInformationSharingConfiguration: CourseInformationSharingConfiguration.COMMUNICATION_AND_MESSAGING, + };Also applies to: 118-118
Line range hint
122-122: Avoid using non-null assertions.- this.controlConfiguration.subject!.next(this.controls); + if (this.controlConfiguration.subject) { + this.controlConfiguration.subject.next(this.controls); + }
Line range hint
280-285: Preferfor...ofloop for better readability and performance in modern JavaScript.- tabs.forEach((tab) => { - route.snapshot.firstChild!.routeConfig!.path = tab; - component.onSubRouteActivate({ controlConfiguration: undefined }); - expect(metisConversationServiceStub).toHaveBeenCalledOnce(); - }); + for (const tab of tabs) { + route.snapshot.firstChild!.routeConfig!.path = tab; + component.onSubRouteActivate({ controlConfiguration: undefined }); + expect(metisConversationServiceStub).toHaveBeenCalledOnce(); + }
Line range hint
301-306: Preferfor...ofloop for iterating over arrays.- tabs.forEach((tab) => { - route.snapshot.firstChild!.routeConfig!.path = tab; - component.onSubRouteActivate({ controlConfiguration: undefined }); - expect(spy).toHaveBeenCalledOnce(); - }); + for (const tab of tabs) { + route.snapshot.firstChild!.routeConfig!.path = tab; + component.onSubRouteActivate({ controlConfiguration: undefined }); + expect(spy).toHaveBeenCalledOnce(); + }
Line range hint
316-319: Usefor...ofloop instead offorEachfor consistency and performance.- tabs.forEach((tab) => { - route.snapshot.firstChild!.routeConfig!.path = tab; - component.onSubRouteActivate({ controlConfiguration: undefined }); - }); + for (const tab of tabs) { + route.snapshot.firstChild!.routeConfig!.path = tab; + component.onSubRouteActivate({ controlConfiguration: undefined }); + }
Line range hint
3-4: Optimize imports by removing unused ones and ensuring that imports used only as types are imported correctly.- import { Observable, Subject, of, throwError } from 'rxjs'; + import { Observable, Subject, of } from 'rxjs'; - import { ComponentFixture, TestBed, fakeAsync, tick } from '@angular/core/testing'; + import { ComponentFixture, TestBed, fakeAsync } from '@angular/core/testing'; - import { HttpHeaders, HttpResponse } from '@angular/common/http'; + import { HttpResponse } from '@angular/common/http'; - import { ActivatedRoute, Params, Router } from '@angular/router'; + import { ActivatedRoute, Router } from '@angular/router'; - import { Course, CourseInformationSharingConfiguration } from 'app/entities/course.model'; + import { Course } from 'app/entities/course.model'; - import { Exercise } from 'app/entities/exercise.model'; + import { Exercise, QuizExercise } from 'app/entities/exercise.model'; - import { Exam } from 'app/entities/exam.model'; + import { Exam, CourseInformationSharingConfiguration } from 'app/entities/exam.model'; - import { TutorialGroup } from 'app/entities/tutorial-group/tutorial-group.model'; + import { TutorialGroup, TutorialGroupsConfiguration } from 'app/entities/tutorial-group/tutorial-group.model';Also applies to: 7-8, 8-9, 18-19, 19-20, 25-26
src/test/javascript/spec/component/shared/navbar.component.spec.ts (4)
Line range hint
137-137: Use template literals for string concatenation.- .mockImplementation((type) => of('Test ' + type.substring(0, 1) + type.substring(1).toLowerCase())); + .mockImplementation((type) => of(`Test ${type.charAt(0)}${type.slice(1).toLowerCase()}`));
Line range hint
261-261: Specify a more precise type instead ofany.Please replace the
anytype with a more specific type to enhance type safety and code maintainability.
Line range hint
676-676: Simplify the computed expression.- window['innerWidth'] = width; + window.innerWidth = width;
Line range hint
1-1: Optimize import statements by using type-only imports where applicable.- import { HttpResponse } from '@angular/common/http'; + import type { HttpResponse } from '@angular/common/http'; - import { ProfileInfo } from 'app/shared/layouts/profiles/profile-info.model'; + import type { ProfileInfo } from 'app/shared/layouts/profiles/profile-info.model'; - import { Exercise, ExerciseType } from 'app/entities/exercise.model'; + import type { Exercise, ExerciseType } from 'app/entities/exercise.model'; - import { Authority } from 'app/shared/constants/authority.constants'; - import { User } from 'app/core/user/user.model'; + import type { Authority } from 'app/shared/constants/authority.constants'; + import type { User } from 'app/core/user/user.model';This change ensures that these imports are used for type checking only and not included in the JavaScript output, optimizing the bundle size.
Also applies to: 6-7, 23-24, 30-31, 33-34
src/main/webapp/app/shared/layouts/navbar/navbar.component.ts (7)
Line range hint
95-95: Consider removing the explicit type annotation where TypeScript can infer it.- private standardizedCompetencySubscription: Subscription; + private standardizedCompetencySubscription;
Line range hint
405-405: Usefor...ofinstead offorEachfor better readability and performance in modern JavaScript.- this.breadcrumbSubscriptions?.forEach((subscription) => subscription.unsubscribe()); + for (const subscription of this.breadcrumbSubscriptions) { + subscription.unsubscribe(); + }
Line range hint
433-433: Use template literals for string concatenation to improve readability.- currentPath += segment + '/'; + currentPath += `${segment}/`;
Line range hint
547-554: These case clauses are redundant and can be removed to clean up the code.case 'mc-question-statistic': case 'dnd-question-statistic': case 'sa-question-statistic': case 'test-exam': case 'participate': case 'overview': - break;
Line range hint
568-568: Use strict equality===for comparisons to avoid unexpected type coercion.- this.isBuildAgentDetails = currentPath.startsWith('/admin/build-agents/') && segment == 'details'; + this.isBuildAgentDetails = currentPath.startsWith('/admin/build-agents/') && segment === 'details';
Line range hint
582-582: Simplify the computed expression without using a string literal.- this.agentName = params['agentName']; + this.agentName = params.agentName;
Line range hint
626-647: Theseelseclauses are unnecessary as the previous branches break early. Simplify the control flow by removing them.if (this.lastRouteUrlSegment === 'user-management') { this.addBreadcrumb(currentPath, segment, false); - } else if (this.lastRouteUrlSegment === 'example-submissions') { + } else if (this.lastRouteUrlSegment === 'example-submissions') { this.addTranslationAsCrumb(currentPath, 'example-submission-editor'); - } else if (this.lastRouteUrlSegment === 'grading') { + } else if (this.lastRouteUrlSegment === 'grading') { this.addTranslationAsCrumb(currentPath, 'grading'); - } else if (this.lastRouteUrlSegment === 'code-editor' && segment === 'new') { + } else if (this.lastRouteUrlSegment === 'code-editor' && segment === 'new') { // This route is bogus and needs to be replaced in the future, display no crumb - } else if (this.lastRouteUrlSegment?.endsWith('-exercises') && segment === 'import') { + } else if (this.lastRouteUrlSegment?.endsWith('-exercises') && segment === 'import') { // This route is bogus and needs to be replaced in the future, display no crumb - } else if (this.lastRouteUrlSegment === 'exercise-groups') { + } else if (this.lastRouteUrlSegment === 'exercise-groups') { // Don't display '<type>-exercises' because it has no associated route - } else if (this.lastRouteUrlSegment === 'exams' && segment === 'import') { + } else if (this.lastRouteUrlSegment === 'exams' && segment === 'import') { // This route is only used internally when opening the exam import modal and therefore shouldn't be displayed. // When opening the exam-update.component, the id of the to be imported exam is appended (-> case `import`). }
| @@ -468,7 +468,7 @@ export class NavbarComponent implements OnInit, OnDestroy { | |||
| default: | |||
| const exercisesMatcher = this.lastRouteUrlSegment?.match(/.+-exercises/); | |||
There was a problem hiding this comment.
Wrap the variable declaration in a block to restrict its scope to the switch clause, preventing potential bugs.
case 'exercises':
+ {
const exercisesMatcher = this.lastRouteUrlSegment?.match(/.+-exercises/);
if (exercisesMatcher) {
this.addResolvedTitleAsCrumb(EntityType.EXERCISE, [Number(segment)], currentPath.replace(`exercises/${exercisesMatcher[0]}`, 'exercises'), 'exercises');
return;
}
+ }Also applies to: 506-506, 617-617
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const exercisesMatcher = this.lastRouteUrlSegment?.match(/.+-exercises/); | |
| case 'exercises': | |
| { | |
| const exercisesMatcher = this.lastRouteUrlSegment?.match(/.+-exercises/); | |
| if (exercisesMatcher) { | |
| this.addResolvedTitleAsCrumb(EntityType.EXERCISE, [Number(segment)], currentPath.replace(`exercises/${exercisesMatcher[0]}`, 'exercises'), 'exercises'); | |
| return; | |
| } | |
| } |
| this.itemSelected | ||
| ? this.router.navigate(['../' + this.sidebarItem.id], { relativeTo: this.route }) | ||
| : this.router.navigate(['./' + this.sidebarItem.id], { relativeTo: this.route }); | ||
| ? this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route }) |
There was a problem hiding this comment.
Consider using template literals for better readability and consistency in string operations.
- this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route })
+ this.router.navigate([`./${this.sidebarItem?.id}`], { relativeTo: this.route })Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ? this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route }) | |
| ? this.router.navigate([`./${this.sidebarItem?.id}`], { relativeTo: this.route }) |
| this.itemSelected | ||
| ? this.router.navigate(['../' + this.sidebarItem.id], { relativeTo: this.route }) | ||
| : this.router.navigate(['./' + this.sidebarItem.id], { relativeTo: this.route }); | ||
| ? this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route }) |
There was a problem hiding this comment.
Use template literals for string operations to maintain consistency and improve readability.
- this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route })
+ this.router.navigate([`./${this.sidebarItem?.id}`], { relativeTo: this.route })Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ? this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route }) | |
| ? this.router.navigate([`./${this.sidebarItem?.id}`], { relativeTo: this.route }) |
| const manageButton = fixture.nativeElement.querySelector('#manage-button'); | ||
| expect(manageButton).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should not display manage button but go to student view button in course management', () => { | ||
| component.course = courseWithShortDescription; | ||
| component.course!.isAtLeastTutor = true; |
There was a problem hiding this comment.
Avoid using non-null assertions.
Using non-null assertions can lead to runtime errors if the values are actually null. Consider checking for null or undefined before usage or using optional chaining.
Also applies to: 104-104
| this.itemSelected | ||
| ? this.router.navigate(['../' + this.sidebarItem.id], { relativeTo: this.route }) | ||
| : this.router.navigate(['./' + this.sidebarItem.id], { relativeTo: this.route }); | ||
| ? this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route }) |
There was a problem hiding this comment.
Adopt template literals for string operations to enhance readability and maintain coding standards.
- this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route })
+ this.router.navigate([`./${this.sidebarItem?.id}`], { relativeTo: this.route })Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ? this.router.navigate(['./' + this.sidebarItem?.id], { relativeTo: this.route }) | |
| ? this.router.navigate([`./${this.sidebarItem?.id}`], { relativeTo: this.route }) |
| }); | ||
|
|
||
| it('should navigate to the item URL on click', async () => { | ||
| const mockFn = jest.fn(); | ||
| component.emitStoreLastSelectedItem = mockFn; | ||
| component.emitStoreAndRefresh = mockFn; |
There was a problem hiding this comment.
Consistency in navigation logic.
The navigation logic in both click handlers is similar but uses different relative paths. Consider refactoring to use a common method or ensure consistency in the navigation strategy.
- expect(navigationArray).toBe('../testId');
+ expect(navigationArray).toBe('./testId');Also applies to: 66-66
Committable suggestion was skipped due low confidence.
| @@ -43,7 +42,7 @@ export class CourseLecturesComponent implements OnInit, OnDestroy { | |||
| ngOnInit() { | |||
| this.isCollapsed = this.courseOverviewService.getSidebarCollapseStateFromStorage('lecture'); | |||
| this.parentParamSubscription = this.route.parent!.params.subscribe((params) => { | |||
There was a problem hiding this comment.
Avoid non-null assertions.
Using non-null assertions (!) can lead to runtime errors if the assumptions about data presence are incorrect. Consider adding checks or using optional chaining.
| @@ -96,9 +97,15 @@ | |||
| return sessionStorage.getItem('sidebar.lastSelectedItem.lecture.byCourse.' + this.courseId); | |||
There was a problem hiding this comment.
Prefer template literals over string concatenation.
Use template literals for better readability and performance.
- return sessionStorage.getItem('sidebar.lastSelectedItem.lecture.byCourse.' + this.courseId);
+ return sessionStorage.getItem(`sidebar.lastSelectedItem.lecture.byCourse.${this.courseId}`);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return sessionStorage.getItem('sidebar.lastSelectedItem.lecture.byCourse.' + this.courseId); | |
| return sessionStorage.getItem(`sidebar.lastSelectedItem.lecture.byCourse.${this.courseId}`); |
| } else if (!lectureId && upcomingLecture) { | ||
| this.router.navigate([upcomingLecture.id], { relativeTo: this.route, replaceUrl: true }); | ||
| } else { | ||
| this.lectureSelected = lectureId ? true : false; |
There was a problem hiding this comment.
Simplify conditional expressions.
Use direct boolean evaluation instead of ternary operations for boolean values.
- this.lectureSelected = lectureId ? true : false;
+ this.lectureSelected = !!lectureId;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| this.lectureSelected = lectureId ? true : false; | |
| this.lectureSelected = !!lectureId; |
| }); | ||
|
|
||
| it('should navigate to the item URL on click', async () => { | ||
| const mockFn = jest.fn(); | ||
| component.emitStoreLastSelectedItem = mockFn; | ||
| component.emitStoreAndRefresh = mockFn; |
There was a problem hiding this comment.
Consistency in navigation logic.
The navigation logic in both click handlers is similar but uses different relative paths. Consider refactoring to use a common method or ensure consistency in the navigation strategy.
- expect(navigationArray).toBe('../testId');
+ expect(navigationArray).toBe('./testId');Also applies to: 88-88
Committable suggestion was skipped due low confidence.
pzdr7
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
rabeatwork
dismissed stale reviews from pzdr7, edkaya, egekurt123, florian-glombik, and JohannesWt
via
9f6ed6f
github-actions
bot
added
the
cypress
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/shared/sidebar/sidebar-card-large.component.spec.ts (1)
Line range hint
24-34: Consider consolidating duplicate setup and teardown hooks.To improve the maintainability and reduce redundancy, you might want to consolidate the repeated setup logic in
beforeEachinto a single block or a helper function.src/test/javascript/spec/component/shared/sidebar/sidebar-card-medium.component.spec.ts (1)
Line range hint
25-35: Consider consolidating duplicate setup and teardown hooks.To improve the maintainability and reduce redundancy, you might want to consolidate the repeated setup logic in
beforeEachinto a single block or a helper function.
| @@ -40,55 +40,55 @@ describe('SidebarCardMediumComponent', () => { | |||
|
|
|||
| it('should have success border class for easy difficulty', () => { | |||
| (component.sidebarItem.difficulty = DifficultyLevel.EASY), fixture.detectChanges(); | |||
| const element: HTMLElement = fixture.nativeElement.querySelector('#test-sidebar-card'); | |||
| const element: HTMLElement = fixture.nativeElement.querySelector('#test-sidebar-card-medium'); | |||
There was a problem hiding this comment.
Correct the misuse of the comma operator in test descriptions.
The comma operator is used incorrectly in the test setup, which might lead to unexpected behavior. Consider using semicolons or separate statements instead.
- (component.sidebarItem.difficulty = DifficultyLevel.EASY), fixture.detectChanges();
+ component.sidebarItem.difficulty = DifficultyLevel.EASY;
+ fixture.detectChanges();Also applies to: 50-50, 57-57
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const element: HTMLElement = fixture.nativeElement.querySelector('#test-sidebar-card-medium'); | |
| const element: HTMLElement = fixture.nativeElement.querySelector('#test-sidebar-card-medium'); | |
| component.sidebarItem.difficulty = DifficultyLevel.EASY; | |
| fixture.detectChanges(); |
| @@ -21,15 +21,15 @@ export class CourseOverviewPage { | |||
| } | |||
|
|
|||
| getExercise(exerciseTitle: string) { | |||
| return cy.contains('#test-sidebar-card', exerciseTitle); | |||
| return cy.contains('#test-sidebar-card-medium', exerciseTitle); | |||
There was a problem hiding this comment.
Consider using template literals for string concatenation to improve readability.
- return cy.contains('#test-sidebar-card-medium', exerciseTitle);
+ return cy.contains(`#test-sidebar-card-medium ${exerciseTitle}`);Committable suggestion was skipped due low confidence.
| @@ -26,7 +26,7 @@ export class ExerciseResultPage { | |||
| } | |||
|
|
|||
| async shouldShowScore(percentage: number) { | |||
| await Commands.reloadUntilFound(this.page, this.page.locator('#submission-result-graded')); | |||
| await Commands.reloadUntilFound(this.page, this.page.locator('jhi-course-exercise-details #submission-result-graded')); | |||
There was a problem hiding this comment.
Consider using template literals for string concatenation to improve readability.
- await expect(this.page.locator('.tab-bar-exercise-details').getByText(`${percentage}%`)).toBeVisible();
+ await expect(this.page.locator(`.tab-bar-exercise-details ${percentage}%`)).toBeVisible();Committable suggestion was skipped due low confidence.
| await expect(courseOverview.getExercise(exercise2.title!)).toBeVisible(); | ||
| await expect(courseOverview.getExercise(exercise3.title!)).toBeVisible(); | ||
| // All quiz exercises should be hidden initially, as the default accordion status is collapsed when there is no due date. | ||
| await expect(courseOverview.getExercise(exercise1.title!)).toBeHidden(); |
There was a problem hiding this comment.
Consider avoiding non-null assertions to prevent potential runtime errors.
- await expect(courseOverview.getExercise(exercise1.title!)).toBeHidden();
- await expect(courseOverview.getExercise(exercise2.title!)).toBeHidden();
- await expect(courseOverview.getExercise(exercise3.title!)).toBeHidden();
- await expect(courseOverview.getExercise(exercise1.title!)).toBeVisible();
- await expect(courseOverview.getExercise(exercise2.title!)).toBeVisible();
- await expect(courseOverview.getExercise(exercise3.title!)).toBeHidden();Also applies to: 31-31, 32-32, 34-34, 35-35, 36-36, 40-40, 41-41, 42-42
Committable suggestion was skipped due low confidence.
| }); | ||
|
|
||
| it('should navigate to the item URL on click', async () => { | ||
| const mockFn = jest.fn(); | ||
| component.emitStoreLastSelectedItem = mockFn; | ||
| component.emitStoreAndRefresh = mockFn; |
There was a problem hiding this comment.
Consistency in navigation logic.
The navigation logic in both click handlers is similar but uses different relative paths. Consider refactoring to use a common method or ensure consistency in the navigation strategy.
- expect(navigationArray).toBe('../testId');
+ expect(navigationArray).toBe('./testId');Also applies to: 65-65
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| component.emitStoreAndRefresh = mockFn; | |
| component.emitStoreAndRefresh = mockFn; | |
| expect(navigationArray).toBe('./testId'); |
florian-glombik
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
The permanently displayed navbar menu allows the user to navigate from any page and quickly identify where they are currently located in Artemis. Additionally, it achieves a consistent design for the student view of Artemis.
Description
The Sidebar is now also visible in nested views.
Exam View and Communication View will be added in other PRs.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Performance Review
Code Review
Manual Tests
Screenshots