General: Replace tab bar with a side navigation bar in the students view
#8083
Conversation
WalkthroughThe recent updates focus on enhancing user experience and interface design across various components of a web application. Key changes include the transition from a tab-based to a sidebar navigation layout in course overview, adjustments in display logic and styling across course-related components, and updates to routing titles. Additionally, improvements are made in the styling of global elements and theme consistency. These changes aim to provide a more intuitive and cohesive user interface. Changes
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 (
|
github-actions
bot
added
tests
client
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| <ng-template #controls> | ||
| <div class="d-flex row course-overview-controls justify-content-end"> | ||
| <div class="d-flex row course-overview-controls text-end"> | ||
| <div class="col" aria-label="Filter Dropdown" ngbDropdown> | ||
| <button class="btn" [class.btn-secondary]="!numberOfAppliedFilters" [class.btn-success]="!!numberOfAppliedFilters" ngbDropdownToggle id="filter-dropdown-button"> | ||
| <fa-icon [icon]="faFilter" /> |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [9-13]
The use of @if for conditional rendering is not standard Angular syntax. Angular uses *ngIf for such purposes. Please replace @if with *ngIf to ensure the template functions correctly.
- @if (categoryFilter.exercisesWithoutCategoriesPresent) {
+ *ngIf="categoryFilter.exercisesWithoutCategoriesPresent"📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-18]
Similarly, the @for directive is used here, which is not recognized in Angular. Angular iterates over collections using *ngFor. Please correct this to use Angular's standard iteration directive.
- @for (category of categoryFilter.exerciseCategories; track category) {
+ *ngFor="let category of categoryFilter.exerciseCategories; trackBy: trackCategory"Note: Ensure you have a trackCategory function implemented in your component to efficiently track items by identity.
There was a problem hiding this comment.
This is not correct. We follow the new angular best practice.
There was a problem hiding this comment.
@MaximilianAnzinger, thank you for pointing out the use of @if and @for as part of the new Angular best practices. I appreciate your patience and the clarification. I'll make sure to follow these guidelines in future reviews.
New Learnings Added
User: MaximilianAnzinger
PR: ls1intum/Artemis#8083
File: src/main/webapp/app/overview/course-statistics/course-statistics.component.html:1-5
Timestamp: 2024-02-27T08:40:15.918Z
Learning: `@if` and `@for` are new and valid Angular syntax replacing *ngIf and *ngFor, and should always be used over the old style in files matching the `src/main/webapp/**/*.html` pattern.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
| @if (ngxDoughnutData && ngxDoughnutColor) { | ||
| <ngx-charts-pie-chart [view]="[350, 350]" [results]="ngxDoughnutData" [scheme]="ngxDoughnutColor" [doughnut]="true"> | ||
| <ng-template #tooltipTemplate let-model="model"> | ||
| <span>{{ model.name | artemisTranslate }}: {{ model.value }}</span> | ||
| </ng-template> | ||
| </ngx-charts-pie-chart> | ||
| } |
There was a problem hiding this comment.
The conditional rendering inside the <ngx-charts-pie-chart> component uses @if, which should be replaced with *ngIf for proper Angular syntax.
- @if (ngxDoughnutData && ngxDoughnutColor) {
+ *ngIf="ngxDoughnutData && ngxDoughnutColor"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.
| @if (ngxDoughnutData && ngxDoughnutColor) { | |
| <ngx-charts-pie-chart [view]="[350, 350]" [results]="ngxDoughnutData" [scheme]="ngxDoughnutColor" [doughnut]="true"> | |
| <ng-template #tooltipTemplate let-model="model"> | |
| <span>{{ model.name | artemisTranslate }}: {{ model.value }}</span> | |
| </ng-template> | |
| </ngx-charts-pie-chart> | |
| } | |
| *ngIf="ngxDoughnutData && ngxDoughnutColor" | |
| <ngx-charts-pie-chart [view]="[350, 350]" [results]="ngxDoughnutData" [scheme]="ngxDoughnutColor" [doughnut]="true"> | |
| <ng-template #tooltipTemplate let-model="model"> | |
| <span>{{ model.name | artemisTranslate }}: {{ model.value }}</span> | |
| </ng-template> | |
| </ngx-charts-pie-chart> |
| <h2 class="text-center">{{ currentRelativeScore }} %</h2> | ||
| <h4 class="text-center">{{ overallPoints }} / {{ reachablePoints }} Pts</h4> | ||
| </div> | ||
| <ngx-charts-pie-chart [view]="[350, 350]" [results]="ngxDoughnutData" [scheme]="ngxDoughnutColor" [doughnut]="true"> | ||
| <ng-template #tooltipTemplate let-model="model"> | ||
| <span>{{ model.name | artemisTranslate }}: {{ model.value }}</span> | ||
| </ng-template> | ||
| </ngx-charts-pie-chart> | ||
|
|
||
| @if (ngxDoughnutData && ngxDoughnutColor) { | ||
| <ngx-charts-pie-chart [view]="[350, 350]" [results]="ngxDoughnutData" [scheme]="ngxDoughnutColor" [doughnut]="true"> | ||
| <ng-template #tooltipTemplate let-model="model"> | ||
| <span>{{ model.name | artemisTranslate }}: {{ model.value }}</span> | ||
| </ng-template> | ||
| </ngx-charts-pie-chart> | ||
| } | ||
| </div> | ||
| <div class="col-md-3 d-flex flex-column align-self-end color-legend-container"> | ||
| @for (exerciseType of ngxDoughnutData; track exerciseType) { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [73-77]
For iterating over ngxDoughnutData, the correct Angular directive *ngFor should be used instead of @for.
- @for (exerciseType of ngxDoughnutData; track exerciseType) {
+ *ngFor="let exerciseType of ngxDoughnutData; trackBy: trackExerciseType"Ensure the trackExerciseType function is defined in your component for tracking purposes.
src/main/webapp/app/overview/course-statistics/course-statistics.component.html
Show resolved
Hide resolved
| this.profileService.getProfileInfo()?.subscribe((profileInfo) => { | ||
| this.inProduction = profileInfo.inProduction; | ||
| }); |
There was a problem hiding this comment.
Fetching profileInfo to set inProduction is a good practice for dynamically adjusting the component's behavior based on the deployment environment. Ensure that error handling is implemented for the subscription to profileService.getProfileInfo() in case the profile information cannot be fetched.
this.profileService.getProfileInfo()?.subscribe({
next: (profileInfo) => {
this.inProduction = profileInfo.inProduction;
},
error: (err) => {
console.error('Failed to fetch profile info:', err);
// Consider setting a default value or handling this scenario appropriately
}
});
rabeatwork
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
rabeatwork
force-pushed
the
enhancement/replace-tabbar-with-sidebar
branch
from
245e8fe
to
678135e
Compare
rabeatwork
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
rabeatwork
force-pushed
the
enhancement/replace-tabbar-with-sidebar
branch
from
678135e
to
6e5d5e4
Compare
src/main/webapp/app/shared/feature-toggle/feature-toggle-hide.directive.ts
Outdated
Show resolved
Hide resolved
rabeatwork
force-pushed
the
enhancement/replace-tabbar-with-sidebar
branch
from
6e5d5e4
to
643cb87
Compare
krusche
added
the
too-long-open
rabeatwork
dismissed stale reviews from MaximilianAnzinger, Predixx, egekurt123, georgyia, MaximilianJG, milljoniaer, and rstief
via
50251ad
rabeatwork
force-pushed
the
enhancement/replace-tabbar-with-sidebar
branch
from
ca48d0e
to
50251ad
Compare
| it('should create sidebar items', () => { | ||
| component.course = { id: 123, lectures: [], exams: [] }; | ||
| const sidebarItems = component.getSidebarItems(); | ||
| expect(sidebarItems.length).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
While the test should create sidebar items correctly checks for the presence of sidebar items, consider adding more detailed assertions to verify that the sidebar items are correctly created based on specific course data attributes.
| public isProduction = true; | ||
| public isTestServer = false; | ||
| public pageTitle: string; | ||
| public sidebarItems: SidebarItem[]; | ||
| public isNotManagementView: boolean; |
There was a problem hiding this comment.
Tip
Codebase Verification
Based on the information gathered, the initialization of isProduction and isTestServer based on the environment within course-overview.component.ts is confirmed. However, there is no evidence of their usage in conditional rendering or execution paths within the same file or elsewhere in the application, as was the concern raised in the review comment. This suggests that while the properties are initialized as intended, their intended functional impact might not be fully implemented or is implemented in a manner not captured by the search patterns used.
Analysis chain
The addition of properties isProduction, isTestServer, pageTitle, sidebarItems, and isNotManagementView is crucial for the component's functionality. Ensure that these properties are initialized and used correctly throughout the component. Particularly, verify that isProduction and isTestServer are set based on the environment and used to conditionally render or execute code paths that are environment-specific.
To further verify the usage of isProduction and isTestServer in conditional code paths within course-overview.component.ts, and potentially in other parts of the application, the following script will be executed:
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the usage of isProduction and isTestServer in conditional code paths
rg --type ts 'isProduction|isTestServer'
Length of output: 462
Script:
#!/bin/bash
# Check for conditional usage of isProduction and isTestServer within course-overview.component.ts
rg --type ts 'if.*isProduction|if.*isTestServer' src/main/webapp/app/overview/course-overview.component.ts
# Search for potential usage of isProduction and isTestServer in other TypeScript files
rg --type ts 'isProduction|isTestServer' --glob '!src/main/webapp/app/overview/course-overview.component.ts'
Length of output: 219
rabeatwork
force-pushed
the
enhancement/replace-tabbar-with-sidebar
branch
from
50251ad
to
badc693
Compare
| getSidebarItems(): SidebarItem[] { | ||
| const sidebarItems = this.getDefaultItems(); | ||
| if (this.course?.lectures) { | ||
| const lecturesItem: SidebarItem = this.getLecturesItems(); | ||
| sidebarItems.splice(-1, 0, lecturesItem); | ||
| } | ||
| if (this.course?.exams && this.hasVisibleExams()) { | ||
| const examsItem: SidebarItem = this.getExamsItems(); | ||
| sidebarItems.unshift(examsItem); | ||
| } | ||
| if (isCommunicationEnabled(this.course)) { | ||
| const communicationItem: SidebarItem = this.getCommunicationItems(); | ||
| sidebarItems.push(communicationItem); | ||
| } | ||
|
|
||
| if (isMessagingEnabled(this.course) || isCommunicationEnabled(this.course)) { | ||
| const messagesItem: SidebarItem = this.getMessagesItems(); | ||
| sidebarItems.push(messagesItem); | ||
| } | ||
|
|
||
| if (this.hasTutorialGroups()) { | ||
| const tutorialGroupsItem: SidebarItem = this.getTutorialGroupsItems(); | ||
| sidebarItems.push(tutorialGroupsItem); | ||
| } | ||
|
|
||
| if (this.hasCompetencies()) { | ||
| const competenciesItem: SidebarItem = this.getCompetenciesItems(); | ||
| sidebarItems.push(competenciesItem); | ||
| if (this.course?.learningPathsEnabled) { | ||
| const learningPathItem: SidebarItem = this.getLearningPathItems(); | ||
| sidebarItems.push(learningPathItem); | ||
| } | ||
| } | ||
|
|
||
| return sidebarItems; |
There was a problem hiding this comment.
The getSidebarItems method and related item generation methods are well-implemented, dynamically constructing the sidebar navigation based on the course's properties. Consider adding unit tests for these methods to ensure their correctness, especially in edge cases.
Would you like assistance in creating unit tests for these methods?
krusche
changed the title
General: Replace tab-bar with a side-navigation-bar in the students viewGeneral: Replace tab bar with a side navigation bar in the students view
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
This Pull Request marks the first of many steps toward the new design of Artemis
Description
The tabbar in the students overview was removed and a new navigation-sidebar was added.
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
Test Coverage
Client
�
Screenshots
Summary by CodeRabbit
New Features
Enhancements
z-index.Style Updates
Refactor
Tests