-
Notifications
You must be signed in to change notification settings - Fork 303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
General
: Allow to switch courses from the course icon
#8669
Conversation
WalkthroughThe changes introduce a new dropdown menu for courses in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Navbar
participant CourseOverviewComponent
participant CourseAccessStorageService
User->>Navbar: Collapse Navbar
Navbar->>CourseOverviewComponent: Update isNavbarCollapsed
CourseOverviewComponent->>CourseAccessStorageService: getLastAccessedCourses(STORAGE_KEY_DROPDOWN)
CourseAccessStorageService-->>CourseOverviewComponent: Return last accessed courses
CourseOverviewComponent-->>User: Render dropdown-courses section with dynamic content
sequenceDiagram
participant User
participant CourseManagementTabBarComponent
participant CourseAccessStorageService
User->>CourseManagementTabBarComponent: Select Course
CourseManagementTabBarComponent->>CourseAccessStorageService: onCourseAccessed(courseId, STORAGE_KEY, MAX_DISPLAYED_RECENTLY_ACCESSED_COURSES_OVERVIEW)
CourseAccessStorageService-->>CourseManagementTabBarComponent: Store course access timestamp
CourseManagementTabBarComponent->>CourseAccessStorageService: onCourseAccessed(courseId, STORAGE_KEY_DROPDOWN, MAX_DISPLAYED_RECENTLY_ACCESSED_COURSES_DROPDOWN)
CourseAccessStorageService-->>CourseManagementTabBarComponent: Store course access timestamp
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (5)
src/main/webapp/app/overview/course-overview.component.ts (1)
Line range hint
110-110
: Remove unnecessary type annotations that are trivially inferred from their initializations to clean up the code.- private courseId: number; - private subscription: Subscription; - course?: Course; - refreshingCourse = false; - private teamAssignmentUpdateListener: Subscription; - private quizExercisesChannel: string;Also applies to: 120-120, 121-121, 122-122, 126-126, 127-127
src/test/javascript/spec/component/course/course-overview.component.spec.ts (4)
Line range hint
124-124
: Specify explicit types instead of usingany
.- const course1Dashboard = { course: course1 } as CourseForDashboardDTO; - const course2Dashboard = { course: course2 } as CourseForDashboardDTO; + const course1Dashboard: CourseForDashboardDTO = { course: course1 }; + const course2Dashboard: CourseForDashboardDTO = { course: course2 };Also applies to: 126-126
Line range hint
130-130
: Avoid using non-null assertions. Consider handling potential null values gracefully.- this.controlConfiguration.subject!.next(this.controls); + if (this.controlConfiguration.subject && this.controls) { + this.controlConfiguration.subject.next(this.controls); + }
Line range hint
289-294
: Prefer usingfor...of
instead offorEach
for better readability and performance in loops.- tabs.forEach((tab) => { + for (const tab of tabs) {Also applies to: 310-315, 325-328
Line range hint
4-5
: Some imports are only used as types. Consider importing them usingimport type
to make this explicit and potentially improve build optimization.- import { Course, CourseInformationSharingConfiguration } from 'app/entities/course.model'; + import type { Course, CourseInformationSharingConfiguration } from 'app/entities/course.model';Also applies to: 8-9, 9-10, 19-20, 20-21, 26-27
changed for loop Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
got added by accident
…' into feature/general/course-switching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall very cool idea, I like the functionality and I think it looks good how you implemented the changes
Apart from my in-code comments, I´d like to request that you increase the surface area that can be clicked to open the dropdown. Why make it only possible to click on the logo and the downward-facing arrow but not on the name of the course? You could also move the downward-facing arrow to the middle then. See attatched screenshot showing the proposed cursor location.
Looking forward to your answers!
accident changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
unnecessary
This reverts commit 605a09f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on ts3, works fine now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes lgtm, reapproval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes lgtm, reapprove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maintainer approve
Checklist
General
Client
Motivation and Context
Many Applications like Slack allow users to navigate between areas just by clicking on the icon. This small feature is a nice little bonus for experienced users to navigate easily between recently accessed courses instead of always navigating to the homepage and searching for the course they want. This feature is also very beneficial for the Test Servers as the developers have too many Courses to choose from but mostly only need 2 - 5 courses to work on. This feature allows them to easily test their features. Exercise Instructors also profit from this change as they tend to have many courses but only need to visit a few of them.
Description
Added the logic and dropdown box to a course icon after navigating to one course. The 5 recently accessed courses will be displayed or if a user is only enrolled in 1 or 2 courses, then these will be displayed. Added a own courseStorageKey for the Dropdown Courses as the normal one only stores 3 Courses in the Recently Accessed Courses. Also adjusted the padding on the Course Icon. Additionally a shared method was extracted to a util Class for Courses. The scenario that the navbar is collapsed or is shortend because the screen is small was considered.
Before:
After:
Steps for Testing
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