Communication: Always show favorite channels
#9510
Conversation
github-actions
bot
added
the
client
cremertim
changed the title
Communication: Do not show channel if it is empty
WalkthroughThe changes enhance the sidebar functionality by introducing a new input property Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 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: 2
🧹 Outside diff range and nitpick comments (16)
src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html (1)
4-10: LGTM! Consider adding a comment for clarity.The addition of the
[sidebarItemShowAlways]input property to the<jhi-sidebar>component aligns well with the PR objective of managing the visibility of sidebar items. The use ofDEFAULT_ALWAYS_SHOWconstant is a good practice for maintaining consistency.Consider adding a brief comment above the
<jhi-sidebar>component to explain the purpose of thesidebarItemShowAlwaysproperty, which could help other developers understand its role in hiding empty channels. For example:<!-- Controls visibility of sidebar items, used to hide empty channels --> <jhi-sidebar [itemSelected]="tutorialGroupSelected" [courseId]="courseId" [sidebarData]="sidebarData" [collapseState]="DEFAULT_COLLAPSE_STATE" [sidebarItemShowAlways]="DEFAULT_ALWAYS_SHOW" />src/main/webapp/app/overview/course-exercises/course-exercises.component.html (1)
4-11: LGTM! Consider adding a comment for clarity.The addition of the
[sidebarItemShowAlways]input property aligns well with the PR objective of hiding empty channels. This change allows for more granular control over which sidebar items are always displayed.Consider adding a brief comment above the
<jhi-sidebar>component to explain the purpose of theDEFAULT_ALWAYS_SHOWconstant and how it affects the visibility of sidebar items. This would enhance code readability and maintainability.src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html (1)
3-12: Consider simplifying the ngClass condition.The restructuring of the div elements looks good and maintains the existing functionality. However, the ngClass directive contains a complex condition:
[ngClass]="{ 'd-none': (!showAddOptions || searchValue?.length !== 0) && !(groupedData[groupKey].entityData | searchFilter: ['title', 'type'] : searchValue)?.length }"This condition might be difficult to understand and maintain. Consider extracting this logic into a component method for better readability and maintainability.
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (1)
35-35: LGTM: New input property declaration looks good.The use of
input.required<SidebarItemShowAlways>()for thesidebarItemShowAlwaysproperty ensures type safety and makes it a required input. This aligns with Angular's latest best practices for declaring input properties.Consider adding a brief JSDoc comment above this line to describe the purpose of this input property. For example:
/** Determines which sidebar items should always be shown */ sidebarItemShowAlways = input.required<SidebarItemShowAlways>();This would enhance code readability and provide context for other developers.
src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1)
35-37: LGTM! Consider adding a comment for clarity.The addition of the
[sidebarItemShowAlways]input property is a good implementation for managing the visibility of sidebar items dynamically. This aligns well with the PR objectives of enhancing the user interface.Consider adding a brief comment above this line to explain the purpose of the
ALWAYS_SHOWconstant and how it affects the sidebar item visibility. This would improve code readability and maintainability.+ <!-- ALWAYS_SHOW constant determines which sidebar items are always visible --> [sidebarItemShowAlways]="ALWAYS_SHOW" [collapseState]="DEFAULT_COLLAPSE_STATE"src/main/webapp/app/shared/sidebar/sidebar.component.html (1)
54-54: LGTM! Consider adding a comment for clarity.The addition of the
[sidebarItemShowAlways]input property to the<jhi-sidebar-accordion>component is correct and aligns with the PR objective of managing the visibility of sidebar items. The implementation uses proper Angular property binding syntax.For improved clarity, consider adding a brief comment above this line explaining the purpose of the
sidebarItemShowAlwaysproperty. For example:<!-- Controls the visibility of sidebar items that should always be shown --> [sidebarItemShowAlways]="sidebarItemShowAlways()"This would help other developers quickly understand the purpose of this property without needing to refer to the component's TypeScript file.
src/main/webapp/app/types/sidebar.ts (1)
28-30: LGTM! Consider adding a brief comment for clarity.The new
SidebarItemShowAlwaystype is well-structured and consistent with existing types. It effectively combines a flexible string-keyed record with specific category records, which aligns with the PR's objective of managing sidebar item visibility.Consider adding a brief comment above the type definition to explain its purpose and usage. For example:
/** * Defines which sidebar items should always be shown for different categories. * Allows for both custom string keys and predefined category keys. */ export type SidebarItemShowAlways = { // ... (rest of the type definition)This would enhance code readability and maintainability.
src/main/webapp/app/overview/course-lectures/course-lectures.component.ts (2)
26-32: LGTM: New constantDEFAULT_ALWAYS_SHOWadded correctly.The new constant
DEFAULT_ALWAYS_SHOWis well-defined and follows the coding guidelines. It's appropriately typed asSidebarItemShowAlwaysand all properties are set tofalse, which aligns with the PR objectives of hiding empty channels by default.Consider adding a brief comment explaining the purpose of this constant, especially its relationship to hiding empty channels. This would enhance code readability and maintainability.
// Default visibility settings for sidebar items, initially set to hide all empty channels const DEFAULT_ALWAYS_SHOW: SidebarItemShowAlways = { // ... (rest of the code remains the same) };
130-130: LGTM: New protected readonly property added correctly.The new protected readonly property
DEFAULT_ALWAYS_SHOWis correctly added to the component class. It follows the coding guidelines and provides access to the constant within the component's template.Consider moving this property declaration to the top of the class, grouping it with other class properties. This would improve code organization and readability. For example:
export class CourseLecturesComponent implements OnInit, OnDestroy { private parentParamSubscription: Subscription; private courseUpdatesSubscription: Subscription; course?: Course; courseId: number; lectureSelected: boolean = true; sidebarData: SidebarData; accordionLectureGroups: AccordionGroups = DEFAULT_UNIT_GROUPS; sortedLectures: Lecture[] = []; sidebarLectures: SidebarCardElement[] = []; isCollapsed: boolean = false; readonly DEFAULT_COLLAPSE_STATE = DEFAULT_COLLAPSE_STATE; protected readonly DEFAULT_ALWAYS_SHOW = DEFAULT_ALWAYS_SHOW; // ... rest of the class }src/main/webapp/app/overview/course-exercises/course-exercises.component.ts (2)
30-36: LGTM: New constant declaration is correct. Consider adding a comment.The new constant
DEFAULT_ALWAYS_SHOWis correctly declared and initialized. It follows the proper naming convention for constants and uses theSidebarItemShowAlwaystype appropriately.Consider adding a brief comment explaining the purpose of this constant, similar to the other constants in this file. For example:
// Default configuration for always showing sidebar items const DEFAULT_ALWAYS_SHOW: SidebarItemShowAlways = { // ... (rest of the code remains the same) };
165-165: LGTM: Class property added correctly. Consider relocating for better organization.The
DEFAULT_ALWAYS_SHOWconstant is correctly added as a protected readonly property to theCourseExercisesComponentclass.For better code organization and readability, consider moving this property declaration to the top of the class, grouping it with other similar properties or constants. For example:
export class CourseExercisesComponent implements OnInit, OnDestroy { private parentParamSubscription: Subscription; private courseUpdatesSubscription: Subscription; private ltiSubscription: Subscription; course?: Course; courseId: number; sortedExercises?: Exercise[]; exerciseForGuidedTour?: Exercise; exerciseSelected: boolean = true; accordionExerciseGroups: AccordionGroups = DEFAULT_UNIT_GROUPS; sidebarData: SidebarData; sidebarExercises: SidebarCardElement[] = []; isCollapsed: boolean = false; readonly DEFAULT_COLLAPSE_STATE = DEFAULT_COLLAPSE_STATE; protected readonly DEFAULT_ALWAYS_SHOW = DEFAULT_ALWAYS_SHOW; isLti: boolean = false; // ... rest of the class }This organization keeps all the class properties and constants together, improving the overall structure of the component.
src/main/webapp/app/shared/sidebar/sidebar.component.ts (1)
35-35: LGTM: New input property added correctly.The
sidebarItemShowAlwaysproperty is correctly implemented using the newinput.required<>()syntax from Angular. This ensures that the property must be provided by the parent component, which aligns with the PR objective of managing the visibility of sidebar items.Consider adding a brief comment explaining the purpose of this property and how it affects the sidebar's behavior. This would improve code readability and maintainability.
src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.ts (1)
31-35: LGTM: New constantDEFAULT_ALWAYS_SHOWadded correctly.The constant is well-defined and follows the coding guidelines. However, for better readability and consistency with the coding guidelines, consider using single quotes for the property names.
Here's a suggested improvement:
const DEFAULT_ALWAYS_SHOW: SidebarItemShowAlways = { 'registered': false, 'all': false, 'further': false, };src/main/webapp/app/overview/course-exams/course-exams.component.ts (2)
27-30: Clarify the purpose ofDEFAULT_ALWAYS_SHOWand consider reducing duplication.The new constant adheres to the naming convention (PascalCase) and uses single quotes for strings, which is good. However:
The purpose of
DEFAULT_ALWAYS_SHOWis not immediately clear from its name or surrounding context. Could you provide a comment explaining its intended use?There seems to be structural duplication between
DEFAULT_ALWAYS_SHOWandDEFAULT_COLLAPSE_STATE. Consider if these could be combined or if there's a way to reduce this duplication.
Line range hint
1-324: Overall assessment: Minor changes with room for clarificationThe changes to this file are minimal and correctly implemented. They adhere to Angular style guidelines and coding conventions. However, there are a few points to consider:
The purpose of the new
DEFAULT_ALWAYS_SHOWconstant is not clear. Adding a comment to explain its intended use would improve code readability.There's potential duplication between
DEFAULT_ALWAYS_SHOWandDEFAULT_COLLAPSE_STATE. Consider if these could be combined or refactored to reduce duplication.The overall structure and logic of the component remain unchanged, which is good for maintaining consistency.
These small improvements would enhance the clarity and maintainability of the code.
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
67-76: Consider adding explanatory comments.While the code changes are clear and well-implemented, it would be beneficial to add brief comments explaining the purpose of the
DEFAULT_ALWAYS_SHOWconstant and theALWAYS_SHOWproperty. This would help future maintainers understand the rationale behind these additions and how they relate to the sidebar's behavior.Also applies to: 108-108
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1 hunks)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (3 hunks)
- src/main/webapp/app/overview/course-exams/course-exams.component.html (1 hunks)
- src/main/webapp/app/overview/course-exams/course-exams.component.ts (2 hunks)
- src/main/webapp/app/overview/course-exercises/course-exercises.component.html (1 hunks)
- src/main/webapp/app/overview/course-exercises/course-exercises.component.ts (3 hunks)
- src/main/webapp/app/overview/course-lectures/course-lectures.component.html (1 hunks)
- src/main/webapp/app/overview/course-lectures/course-lectures.component.ts (3 hunks)
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html (1 hunks)
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.ts (3 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (5 hunks)
- src/main/webapp/app/shared/sidebar/sidebar.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar.component.ts (2 hunks)
- src/main/webapp/app/types/sidebar.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (15)
src/main/webapp/app/overview/course-conversations/course-conversations.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/overview/course-conversations/course-conversations.component.ts (1)
src/main/webapp/app/overview/course-exams/course-exams.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/overview/course-exams/course-exams.component.ts (1)
src/main/webapp/app/overview/course-exercises/course-exercises.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/overview/course-exercises/course-exercises.component.ts (1)
src/main/webapp/app/overview/course-lectures/course-lectures.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/overview/course-lectures/course-lectures.component.ts (1)
src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.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/overview/course-tutorial-groups/course-tutorial-groups.component.ts (1)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.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/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (1)
src/main/webapp/app/shared/sidebar/sidebar.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/shared/sidebar/sidebar.component.ts (1)
src/main/webapp/app/types/sidebar.ts (1)
🔇 Additional comments (28)
src/main/webapp/app/overview/course-lectures/course-lectures.component.html (2)
4-10: New input property added to<jhi-sidebar>componentThe addition of the
[sidebarItemShowAlways]="DEFAULT_ALWAYS_SHOW"property to the<jhi-sidebar>component aligns with the PR objectives to enhance the user interface by managing the visibility of sidebar items. This change allows for more dynamic control over which items are always displayed in the sidebar.Correct usage of new Angular syntax
The template correctly uses the new Angular syntax
@ifinstead of*ngIf, adhering to the provided coding guidelines.
Line range hint
1-27: Summary of changes and their impactThe changes in this file successfully implement the new
[sidebarItemShowAlways]input property for the<jhi-sidebar>component. This addition aligns with the PR objectives to enhance the user interface by allowing more control over the visibility of sidebar items. The template also correctly uses the new Angular syntax@ifinstead of*ngIf, adhering to the provided coding guidelines.These changes contribute to the overall goal of removing empty communication channels from the sidebar, as outlined in the PR objectives. The new property will likely be used in conjunction with other components to determine which items should always be shown, regardless of their content status.
src/main/webapp/app/overview/course-exams/course-exams.component.html (1)
2-2: Correct usage of new Angular syntaxThe template correctly uses the new
@ifsyntax instead of*ngIfas per the coding guidelines. This is applied consistently for both the outer and inner conditional blocks.Also applies to: 13-13, 17-17
src/main/webapp/app/overview/course-exercises/course-exercises.component.html (1)
Line range hint
1-26: Excellent adherence to coding guidelines!The template correctly uses the new Angular syntax
@ifinstead of*ngIf, as specified in the coding guidelines. This is evident in two instances within the file.src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html (4)
1-2: LGTM: New syntax and improved conditional rendering.The changes here are well-implemented:
- The use of
@forinstead of*ngForadheres to the new Angular syntax as per our coding guidelines.- The conditional rendering logic with
@ifeffectively addresses the PR objective of hiding empty channels while allowing for exceptions through thesidebarItemShowAlways()function.These changes contribute to a cleaner UI by removing empty communication channels from the sidebar.
Also applies to: 47-47
25-44: LGTM: Improved content rendering logic.The changes to the accordion item content rendering are well-implemented:
- The use of
@ifto conditionally render content only when there's data to display aligns perfectly with the PR objective of hiding empty channels.- The existing accordion functionality is maintained through the use of
ngbCollapse.These changes contribute to a more efficient and cleaner UI by ensuring that empty content is not rendered unnecessarily.
28-40: LGTM: Correct implementation of new @for syntax.The changes in this section are well-implemented:
- The use of
@forinstead of*ngForadheres to the new Angular syntax as per our coding guidelines.- The track by expression and the use of the
lastvariable for conditional styling are correctly implemented, maintaining the existing functionality.This change contributes to keeping the codebase up-to-date with the latest Angular practices.
30-38: LGTM: Unchanged jhiSidebarCard implementation.The implementation of the jhiSidebarCard directive remains unchanged:
- It correctly binds multiple input properties: size, itemSelected, sidebarType, and sidebarItem.
- The output event (onUpdateSidebar) is properly emitted.
This part of the code maintains the existing functionality without introducing any changes, which is appropriate given the focus of this PR.
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (2)
1-12: LGTM: Import and type declaration changes look good.The addition of
inputfrom '@angular/core' andSidebarItemShowAlwaysfrom 'app/types/sidebar' enhances type safety and aligns with Angular's latest input signal syntax. These changes contribute to better code organization and maintainability.
56-56: Clarify the switch from sessionStorage to localStorage.The change from
sessionStoragetolocalStoragefor storing the collapse state will persist this information across browser sessions. While this can improve user experience by maintaining preferences, it's important to consider:
- Is this the intended behavior?
- Are there any privacy implications to consider?
- Should we provide a way for users to clear this persistent state?
Please confirm if this change aligns with the project's requirements and user expectations. If confirmed, consider adding a brief comment explaining the rationale behind this change.
src/main/webapp/app/overview/course-conversations/course-conversations.component.html (2)
Line range hint
18-18: Excellent use of new Angular syntax!The implementation consistently uses the new
@ifdirective instead of*ngIfthroughout the file. This adheres to the provided coding guidelines and represents best practices in modern Angular development.Also applies to: 28-28, 40-40, 51-51
Line range hint
1-65: Overall, excellent implementation addressing PR objectives.The changes in this file effectively implement the PR objectives of enhancing the user interface for the course conversations component. Key points:
- The new
[sidebarItemShowAlways]property allows for dynamic control of sidebar item visibility.- Consistent use of new Angular syntax (
@if) throughout the file, adhering to coding guidelines.- The structure and conditional rendering logic align well with the described functionality in the PR summary.
The implementation is clean, follows best practices, and should effectively remove empty communication channels from the sidebar as intended.
src/main/webapp/app/shared/sidebar/sidebar.component.html (1)
Line range hint
1-80: Excellent use of new Angular syntax!The template consistently uses the new
@ifand@fordirectives instead of the older*ngIfand*ngForsyntax. This aligns perfectly with the provided coding guidelines and demonstrates a commitment to using modern Angular best practices.Great job on adopting the new syntax throughout the template. This will improve readability and maintainability of the code.
src/main/webapp/app/types/sidebar.ts (1)
Line range hint
1-30: Overall, the changes look good and align with the PR objectives.The addition of the
SidebarItemShowAlwaystype is well-integrated into the existing type definitions. It provides a flexible structure for managing the visibility of sidebar items, which is consistent with the PR's goal of enhancing the user interface by removing empty communication channels.The file adheres to the specified coding guidelines, including:
- Using PascalCase for types
- Following Angular style guide recommendations
- Maintaining consistent naming conventions
No further issues or improvements are identified in this file.
src/main/webapp/app/overview/course-lectures/course-lectures.component.ts (2)
7-7: LGTM: Import statement correctly updated.The import statement has been properly updated to include the new
SidebarItemShowAlwaystype. This change follows the coding guidelines and is consistent with the new constant being introduced.
Line range hint
1-131: Summary: Changes align with PR objectives and follow coding guidelines.The modifications to this file contribute effectively to the PR's goal of hiding empty communication channels in the sidebar. The new
DEFAULT_ALWAYS_SHOWconstant and its corresponding class property provide the necessary structure to manage the visibility of sidebar items.Key points:
- The changes follow Angular and project-specific coding guidelines.
- The new constant and property are correctly implemented and typed.
- The modifications lay the groundwork for dynamic sidebar item visibility management.
To ensure these changes are properly utilized, please verify that:
- The
DEFAULT_ALWAYS_SHOWconstant is used in the template or other components to control sidebar item visibility.- The hiding of empty channels is implemented correctly in the related HTML template.
You can use the following script to check for the usage of
DEFAULT_ALWAYS_SHOWin other files:This will help ensure that the changes are fully implemented across the necessary components.
src/main/webapp/app/overview/course-exercises/course-exercises.component.ts (2)
10-10: LGTM: Import statement updated correctly.The import statement has been properly updated to include the new
SidebarItemShowAlwaystype from the 'app/types/sidebar' module. This change follows the Angular style guide for imports and maintains consistency with the existing code.
Line range hint
1-165: Summary: Changes align with PR objectives, with minor suggestions for improvement.The changes made to this file contribute to the PR's objective of enhancing the user interface for communication channels. The introduction of the
DEFAULT_ALWAYS_SHOWconstant and its integration as a class property provide a foundation for managing the visibility of sidebar items, which is crucial for removing empty communication channels from the sidebar.The code changes are generally well-implemented, following Angular best practices and maintaining consistency with the existing codebase. Minor suggestions have been made for improved documentation and code organization.
To fully achieve the PR objectives:
- Ensure that this new constant is utilized in the component's template or in other methods to control the visibility of sidebar items.
- Verify that empty channels are indeed hidden in the UI when using this new configuration.
- Test the functionality across different scenarios as outlined in the PR description.
To verify the implementation, please run the following script:
This script will help ensure that the new constant is being used appropriately and that there are no remaining TODO items related to this feature.
✅ Verification successful
Verification Successful:
DEFAULT_ALWAYS_SHOWis correctly implemented and utilized
DEFAULT_ALWAYS_SHOWis properly used incourse-exercises.component.htmlas expected.- Consistent usage of
DEFAULT_ALWAYS_SHOWacross related components ensures uniform behavior.- No TODO comments related to sidebar visibility were found, indicating completeness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of DEFAULT_ALWAYS_SHOW in the component's template and related files. # Test 1: Check if DEFAULT_ALWAYS_SHOW is used in the component's template echo "Checking usage in component template:" rg --type html "\[sidebarItemShowAlways\]" src/main/webapp/app/overview/course-exercises/course-exercises.component.html # Test 2: Check for any other usages of DEFAULT_ALWAYS_SHOW in related files echo "Checking usage in related files:" rg "DEFAULT_ALWAYS_SHOW" src/main/webapp/app/overview/ # Test 3: Verify if there are any TODO comments related to this feature echo "Checking for related TODO comments:" rg "TODO.*sidebar.*visibility" src/main/webapp/app/overview/Length of output: 2434
src/main/webapp/app/shared/sidebar/sidebar.component.ts (2)
6-6: LGTM: Import statement updated correctly.The addition of
SidebarItemShowAlwaysto the import statement is consistent with the coding guidelines and Angular best practices. This change is necessary to support the new input property.
Line range hint
1-185: Verify the usage of the newsidebarItemShowAlwaysproperty.While the new
sidebarItemShowAlwaysproperty has been correctly added to the component, its usage is not evident in the current changes. To ensure the PR objective of hiding empty channels is met:
- Verify that the property is being used in the component's template or passed to child components to control the visibility of sidebar items.
- Consider adding logic in the component class to filter or process the sidebar items based on this property.
- Update the
ngOnChangesmethod to react to changes in this property if necessary.To help verify the property's usage, you can run the following command:
This will help ensure that the new property is being utilized as intended to meet the PR objectives.
✅ Verification successful
Verified the usage of the
sidebarItemShowAlwaysproperty.The
sidebarItemShowAlwaysproperty is properly defined in bothsidebar.component.tsandsidebar-accordion.component.tsand is utilized in their respective HTML templates to control the visibility of sidebar items. This ensures that empty channels are appropriately hidden, meeting the PR's objective.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of sidebarItemShowAlways in the component's template and related files rg --type html 'sidebarItemShowAlways' src/main/webapp/app/shared/sidebar/ rg --type typescript 'sidebarItemShowAlways' src/main/webapp/app/shared/sidebar/Length of output: 562
Script:
#!/bin/bash # Search for usage of sidebarItemShowAlways in the component's TypeScript files rg --type ts 'sidebarItemShowAlways' src/main/webapp/app/shared/sidebar/Length of output: 350
src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.ts (3)
15-15: LGTM: Import statement updated correctly.The addition of
SidebarItemShowAlwaysto the import statement is consistent with the coding guidelines and is correctly placed with other related imports.
58-58: LGTM:DEFAULT_ALWAYS_SHOWadded as readonly property.The addition of
DEFAULT_ALWAYS_SHOWas a readonly property in theCourseTutorialGroupsComponentclass is correct and follows the coding guidelines.
Line range hint
1-324: Summary: Changes align with PR objectives and coding guidelines.The modifications to this file, including the addition of
DEFAULT_ALWAYS_SHOWconstant and its integration as a readonly property in theCourseTutorialGroupsComponentclass, contribute to the PR's goal of enhancing the user interface for communication channels in the sidebar. These changes provide a configuration option for controlling the visibility of sidebar items, which is consistent with the objective of removing empty communication channels from the sidebar.The changes adhere to the coding guidelines and best practices for Angular development. The only suggestion for improvement is a minor code style change in the constant definition.
Overall, these changes appear to be a solid foundation for implementing the feature described in the PR objectives.
src/main/webapp/app/overview/course-exams/course-exams.component.ts (1)
65-65: LGTM: Readonly property addition is correct.The addition of
DEFAULT_ALWAYS_SHOWas a readonly property is correct and follows best practices:
- It uses camelCase naming convention for properties.
- It's marked as readonly, preventing accidental modifications.
- Its placement next to
DEFAULT_COLLAPSE_STATEis logical and improves code organization.src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (4)
16-16: LGTM: Import statement updated correctly.The addition of
SidebarItemShowAlwaysto the import statement is consistent with its usage in the new constant and follows Angular style guidelines.
57-62: LGTM: Default collapse states updated.The changes to
DEFAULT_COLLAPSE_STATEalter the initial visibility of general and favorite channels in the sidebar. This modification is consistent with the component's logic and likely reflects an intentional design decision. Ensure that this change aligns with the expected user experience.
67-76: LGTM: New constant for sidebar item visibility added.The
DEFAULT_ALWAYS_SHOWconstant provides a clear structure for managing the visibility of sidebar items. It follows the correct naming convention and uses the appropriate type. The default values seem logical, ensuring that important channels like general, group chats, direct messages, and favorites are always visible.
108-108: LGTM: New readonly property added to component.The
ALWAYS_SHOWreadonly property provides a clean way for the component to access the always-show configuration. It follows coding guidelines and ensures immutability, which is a good practice for configuration properties.
src/main/webapp/app/overview/course-exams/course-exams.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-exams/course-exams.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-exercises/course-exercises.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-lectures/course-lectures.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-lectures/course-lectures.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (3)
35-35: LGTM: New input property looks good, with a minor suggestionThe new
sidebarItemShowAlwaysinput property is well-implemented using the new Angular 16 input syntax. It aligns with the PR objectives and follows the coding guidelines.Consider adding a brief JSDoc comment to explain the purpose of this property, which will improve code documentation:
/** Determines which sidebar items should always be shown */ sidebarItemShowAlways = input.required<SidebarItemShowAlways>();
56-57: LGTM: Storage mechanism change looks good, with a minor suggestionThe switch from
sessionStoragetolocalStorageis a good improvement, ensuring that the collapse state persists across browser sessions. This change aligns well with the PR objectives.To improve code readability, consider extracting the storage key into a constant:
private readonly STORAGE_KEY = 'sidebar.accordion.collapseState'; // ... setStoredCollapseState() { const key = `${this.STORAGE_KEY}.${this.storageId}.byCourse.${this.courseId}`; const storedCollapseState: string | null = localStorage.getItem(key); if (storedCollapseState) this.collapseState = JSON.parse(storedCollapseState); }This change would make the code more maintainable and reduce the risk of typos when using the same key in multiple places.
83-83: LGTM: Storage mechanism change is consistentThe change from
sessionStoragetolocalStorageintoggleGroupCategoryCollapseis consistent with the earlier modification and aligns well with the PR objectives.For consistency and improved readability, apply the same suggestion as before:
toggleGroupCategoryCollapse(groupCategoryKey: string) { this.collapseState[groupCategoryKey] = !this.collapseState[groupCategoryKey]; const key = `${this.STORAGE_KEY}.${this.storageId}.byCourse.${this.courseId}`; localStorage.setItem(key, JSON.stringify(this.collapseState)); }This change would use the same
STORAGE_KEYconstant suggested earlier, improving maintainability and reducing the risk of inconsistencies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1 hunks)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (3 hunks)
- src/main/webapp/app/overview/course-exams/course-exams.component.html (1 hunks)
- src/main/webapp/app/overview/course-exams/course-exams.component.ts (2 hunks)
- src/main/webapp/app/overview/course-exercises/course-exercises.component.html (1 hunks)
- src/main/webapp/app/overview/course-exercises/course-exercises.component.ts (3 hunks)
- src/main/webapp/app/overview/course-lectures/course-lectures.component.html (1 hunks)
- src/main/webapp/app/overview/course-lectures/course-lectures.component.ts (3 hunks)
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html (1 hunks)
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.ts (3 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.html
- src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
- src/main/webapp/app/overview/course-exams/course-exams.component.html
- src/main/webapp/app/overview/course-exams/course-exams.component.ts
- src/main/webapp/app/overview/course-exercises/course-exercises.component.html
- src/main/webapp/app/overview/course-exercises/course-exercises.component.ts
- src/main/webapp/app/overview/course-lectures/course-lectures.component.html
- src/main/webapp/app/overview/course-lectures/course-lectures.component.ts
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.ts
- src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (2)
1-12: LGTM: Import changes look goodThe import changes are well-organized and align with the coding guidelines. The addition of
SidebarItemShowAlwaysis consistent with the PR objectives for improving sidebar functionality.
Line range hint
1-92: Overall assessment: Good changes with minor suggestions for improvementThe changes in this file align well with the PR objectives of improving sidebar functionality. The switch from
sessionStoragetolocalStoragefor storing collapse states is a good improvement, ensuring persistence across browser sessions. The new input propertysidebarItemShowAlwaysis well-implemented using the new Angular 16 input syntax.Key points:
- Import changes are well-organized and consistent with coding guidelines.
- The new input property enhances sidebar functionality as intended.
- Storage mechanism changes are consistent and improve user experience.
Minor suggestions have been made to improve code readability and maintainability:
- Adding a JSDoc comment for the new input property.
- Extracting the storage key into a constant for better maintainability.
These changes look good overall, and implementing the minor suggestions would further enhance the code quality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (4)
35-35: LGTM: New input property added using modern Angular syntax.The
sidebarItemAlwaysShowproperty is correctly implemented using the new Angular 16 input syntax and is properly typed. This change supports the PR objective of improving sidebar functionality.Consider adding a brief JSDoc comment to explain the purpose of this property, enhancing code readability and maintainability.
56-56: LGTM: Storage mechanism updated for better persistence.The change from
sessionStoragetolocalStorageis appropriate and aligns with the PR objective of persisting UI state across sessions. The key structure allows for course-specific settings.Consider extracting the storage key construction to a separate method for better reusability and maintainability, as it's used in multiple places.
83-83: LGTM: Consistent update of storage mechanism.The change to
localStorageintoggleGroupCategoryCollapseis consistent with the earlier modification and ensures persistence of the collapse state across sessions.As mentioned earlier, consider extracting the storage key construction to a separate method. This would improve code maintainability and reduce the risk of inconsistencies. For example:
private getStorageKey(): string { return `sidebar.accordion.collapseState.${this.storageId}.byCourse.${this.courseId}`; }Then use it in both
setStoredCollapseStateandtoggleGroupCategoryCollapse:localStorage.getItem(this.getStorageKey()); localStorage.setItem(this.getStorageKey(), JSON.stringify(this.collapseState));
Line range hint
1-83: Summary: Improved sidebar functionality with persistent state.The changes in this file successfully implement the PR objectives:
- Introduction of
sidebarItemAlwaysShowproperty for dynamic visibility management.- Switching from
sessionStoragetolocalStoragefor persistent collapse state across sessions.- Consistent implementation of storage mechanism changes.
These modifications enhance the user experience by maintaining sidebar state and allowing for more flexible control over item visibility. The code adheres to Angular best practices and TypeScript conventions.
To further improve the component:
- Consider implementing a caching mechanism or debounce function for frequent localStorage operations to optimize performance.
- Evaluate the possibility of using Angular's
ChangeDetectionStrategy.OnPushfor potential performance benefits, especially if this component is used in a large application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1 hunks)
- src/main/webapp/app/overview/course-exams/course-exams.component.html (1 hunks)
- src/main/webapp/app/overview/course-exercises/course-exercises.component.html (1 hunks)
- src/main/webapp/app/overview/course-lectures/course-lectures.component.html (1 hunks)
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (4 hunks)
- src/main/webapp/app/shared/sidebar/sidebar.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar.component.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.html
- src/main/webapp/app/overview/course-exams/course-exams.component.html
- src/main/webapp/app/overview/course-exercises/course-exercises.component.html
- src/main/webapp/app/overview/course-lectures/course-lectures.component.html
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html
- src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html
- src/main/webapp/app/shared/sidebar/sidebar.component.html
- src/main/webapp/app/shared/sidebar/sidebar.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (1)
1-12: LGTM: Import changes are well-organized and support new functionality.The new imports, including
SidebarItemShowAlways, are correctly added and grouped. This aligns with the coding guidelines and supports the implementation of the newsidebarItemAlwaysShowproperty.
sachmii
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
github-actions
bot
removed
the
deployment-error
JanaNF
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Cathy0123456789
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS1.
Favorites tab was uncollapsed when opening course the first time. Creating/removing exercises, lectures and exams created/removed new channels.
After adding channels as favorites and collapsing the favorite tab, logging out, closing the browser tab, reopening it, logging in and navigating to the course, favorites tab was still collapsed. If that's meant to be, then everything works fine from my side.
(cherry picked from commit 7a14167)
Checklist
General
Client
Motivation and Context
Empty channels consume a lot of space in the sidebar for the fact that you cannot click on them
Description
Default open favorites tab on a new tab
removed empty channels
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
Code Review
Manual Tests
Screenshots
Lectures and exams are missing because they are empty
Summary by CodeRabbit
New Features
[sidebarItemAlwaysShow]for improved configurability.Bug Fixes
Improvements
SidebarItemShowAlwaysto manage sidebar item visibility settings.