Skip to content
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

Communication: Add profile picture to sidebar element and conversation header #9719

Merged
merged 3 commits into from
Nov 10, 2024

Conversation

asliayk
Copy link
Contributor

@asliayk asliayk commented Nov 9, 2024

Checklist

General

Client

  • I strictly followed the client coding and design guidelines.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added multiple screenshots/screencasts of my UI changes.

Motivation and Context

  • In the current sidebar design, group and direct messages share the same icon as channels, causing confusion. Direct messages display the channel icon instead of profile pictures, making it harder for users to distinguish between channels, group chats, and direct messages.
  • The conversation header does not display the profile picture of the person being chatted with, unlike in Slack.

Description

  • Added a group icon for group chats in the sidebar
  • Displayed the relevant profile picture for direct chats in the sidebar, differentiating them from channels and group chats
  • Updated the conversation header to show the relevant profile picture for direct chats

Steps for Testing

Prerequisites:

  • 1 Instructor/Student
  • 1 Course with Communication enabled
  1. Log in to Artemis
  2. Navigate to Communication section of a course
  3. Create a direct chat and a group chat, notice their updated icons on the sidebar
  4. Open a direct chat, notice the updated conversation header with a profile picture.

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

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Line Coverage Confirmation (assert/expect)
one-to-one-chat.model.ts 90.9% ✅ ❌
conversation-detail-dialog.component.ts 97.05% ✅ ❌
conversation-header.component.ts 95.58% ✅ ❌
group-chat-icon.component.ts not found (deleted)
sidebar-card-item.component.ts 100%
sidebar.component.ts 91.66% ✅ ❌

Screenshots

updated conversation header
image

updated sidebar elements
image

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced enhanced handling for one-to-one chat conversations, improving user experience in identifying participants.
    • Updated visual representation of group chats with a new icon.
    • Added profile picture display for one-to-one conversations in the sidebar.
  • Bug Fixes

    • Removed deprecated group chat icon component, streamlining the conversation interface.
  • Documentation

    • Updated test suites to reflect the removal of the group chat icon and ensure coverage of new functionalities.
  • Chores

    • Simplified component dependencies and improved test configurations for better maintainability.

@asliayk asliayk added tests client Pull requests that update TypeScript code. (Added Automatically!) component:Communication communication Pull requests that affect the corresponding module labels Nov 9, 2024
@asliayk asliayk self-assigned this Nov 9, 2024
@asliayk asliayk requested a review from a team as a code owner November 9, 2024 13:11
@github-actions github-actions bot removed the communication Pull requests that affect the corresponding module label Nov 9, 2024
Copy link

coderabbitai bot commented Nov 9, 2024

Walkthrough

The changes in this pull request include the introduction of a new function getAsOneToOneChatDTO in the one-to-one-chat.model.ts file, which enhances type safety for handling one-to-one chat instances. Additionally, the GroupChatIconComponent has been removed from the CourseConversationsModule, and its usage has been replaced with a Font Awesome icon in various components. The sidebar and conversation header components have been updated to conditionally render user profile information based on the conversation type, improving the user interface.

Changes

File Change Summary
src/main/webapp/app/entities/metis/conversation/one-to-one-chat.model.ts Method added: `getAsOneToOneChatDTO(conversation: ConversationDTO
src/main/webapp/app/overview/course-conversations/course-conversations.module.ts Component removed: GroupChatIconComponent from CourseConversationsModule, incorrect import of ProfilePictureComponent corrected to declaration.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html HTML modified: Replaced <jhi-group-chat-icon /> with <fa-icon [icon]="faPeopleGroup" size="xs" />, simplified conditional check for group chat.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts Property added: readonly faPeopleGroup initialized with imported icon.
src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.html HTML modified: Conditional rendering for profile picture added, replaced group chat icon with Font Awesome icon, simplified group chat state handling.
src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts Property added: otherUser?: ConversationUserDTO, Method added: getOtherUser(), Method signature updated for getAsOneToOneChat.
src/main/webapp/app/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.html File deleted: group-chat-icon.component.html.
src/main/webapp/app/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.ts File deleted: GroupChatIconComponent.
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html HTML modified: Conditional rendering logic updated for user profile information based on otherUser.
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts Property added: otherUser: any, Property added: readonly faPeopleGroup = faPeopleGroup.
src/main/webapp/app/shared/sidebar/sidebar.component.html HTML modified: Replaced <fa-icon [icon]="faUsers"> with <fa-icon [icon]="faPeopleGroup">.
src/main/webapp/app/shared/sidebar/sidebar.component.ts Property updated: readonly faUsers replaced with readonly faPeopleGroup, various properties initialized using output<void>() or input.required<>().
src/main/webapp/app/shared/sidebar/sidebar.module.ts New import added: ProfilePictureComponent from app/shared/profile-picture/profile-picture.component.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/conversation-add-users-dialog.component.spec.ts Component removed from declarations: GroupChatIconComponent.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts Import added: FontAwesomeModule, Import removed: GroupChatIconComponent.
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts Variable renamed: MockComponent(GroupChatIconComponent) to MockComponent(ProfilePictureComponent), New test added for getOtherUser().
src/test/javascript/spec/component/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.spec.ts File deleted: group-chat-icon.component.spec.ts.
src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts Variable added: let sidebarItemMock: any;, New tests added for group icon and other user properties.

Possibly related PRs

Suggested labels

ready to merge, small, user interface

Suggested reviewers

  • edkaya
  • JohannesStoehr
  • HawKhiem
  • pzdr7
  • SimonEntholzer
  • rabeatwork

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (9)
src/main/webapp/app/entities/metis/conversation/one-to-one-chat.model.ts (1)

19-24: Add JSDoc documentation for better maintainability.

While the function implementation is correct and follows TypeScript best practices, adding JSDoc documentation would improve maintainability and provide better IDE support.

Consider adding documentation like this:

+/**
+ * Safely converts a ConversationDTO to OneToOneChatDTO if possible.
+ * @param conversation - The conversation to convert
+ * @returns The conversation as OneToOneChatDTO if it's a one-to-one chat, undefined otherwise
+ */
export function getAsOneToOneChatDTO(conversation: ConversationDTO | undefined): OneToOneChatDTO | undefined {
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts (1)

Line range hint 18-24: Consider adding ARIA label for the group icon

Since you're replacing the group chat icon component with a Font Awesome icon, ensure that the template includes proper ARIA labels for accessibility.

Example usage in the template:

// In the template where the icon is used:
<fa-icon 
  [icon]="faPeopleGroup" 
  size="xs"
  [attr.aria-label]="'artemis.conversation.groupChat' | translate"
/>
src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.html (1)

19-32: Enhance accessibility and maintainability of the profile picture component.

While the implementation aligns with the PR objectives, consider the following improvements:

  1. Add accessibility attributes for screen readers
  2. Extract hardcoded values to component constants
  3. Handle profile picture loading errors

Apply these improvements:

 @if (getAsOneToOneChat(activeConversation) && otherUser) {
     <jhi-profile-picture
         [imageSizeInRem]="'2'"
         [fontSizeInRem]="'0.9'"
         [imageId]="'sidebar-profile-picture'"
         [defaultPictureId]="'sidebar-default-profile-picture'"
         [isGray]="false"
         [authorId]="otherUser.id"
         [authorName]="otherUser.name"
         class="me-2"
         [imageUrl]="otherUser.imageUrl"
         [isEditable]="false"
+        [attr.aria-label]="'Profile picture of ' + otherUser.name"
+        (error)="handleProfilePictureError($event)"
     >
     </jhi-profile-picture>
 }

Consider moving these values to the component:

// conversation-header.component.ts
readonly PROFILE_PICTURE_CONFIG = {
  imageSize: '2',
  fontSize: '0.9',
  imageId: 'sidebar-profile-picture',
  defaultPictureId: 'sidebar-default-profile-picture'
} as const;
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html (1)

57-70: LGTM! Consider adding accessibility attributes.

The profile picture implementation looks good and aligns with the PR objectives. However, consider these improvements:

Add an aria-label for better accessibility:

                    @if (otherUser) {
                        <jhi-profile-picture
                            [imageSizeInRem]="'1.1'"
                            [fontSizeInRem]="'0.5'"
                            [imageId]="'sidebar-profile-picture'"
                            [defaultPictureId]="'sidebar-default-profile-picture'"
                            [isGray]="false"
                            [authorId]="otherUser.id"
                            [authorName]="otherUser.name"
                            [imageUrl]="otherUser.imageUrl"
                            [isEditable]="false"
+                           [attr.aria-label]="'Profile picture of ' + otherUser.name"
                        >
                        </jhi-profile-picture>

Consider making the image size configurable via component input to allow for different sizes in different contexts:

// In the component class
@Input() profilePictureSizeRem = 1.1;
@Input() profilePictureFontSizeRem = 0.5;

Then in the template:

                        <jhi-profile-picture
-                           [imageSizeInRem]="'1.1'"
-                           [fontSizeInRem]="'0.5'"
+                           [imageSizeInRem]="profilePictureSizeRem"
+                           [fontSizeInRem]="profilePictureFontSizeRem"
src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts (1)

Line range hint 98-113: Consider enhancing modal subscription cleanup.

While the component generally handles memory leaks well, the modal subscriptions in openAddUsersDialog and openConversationDetailDialog could benefit from additional cleanup.

Consider storing and cleaning up modal references:

+    private modalRef?: NgbModalRef;

     openAddUsersDialog(event: MouseEvent) {
         event.stopPropagation();
-        const modalRef: NgbModalRef = this.modalService.open(ConversationAddUsersDialogComponent, defaultFirstLayerDialogOptions);
+        this.modalRef = this.modalService.open(ConversationAddUsersDialogComponent, defaultFirstLayerDialogOptions);
-        modalRef.componentInstance.course = this.course;
+        this.modalRef.componentInstance.course = this.course;
         // ... rest of the method
     }

     ngOnDestroy() {
+        if (this.modalRef) {
+            this.modalRef.close();
+        }
         this.ngUnsubscribe.next();
         this.ngUnsubscribe.complete();
     }

Also applies to: 117-134

src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/conversation-add-users-dialog.component.spec.ts (1)

44-44: LGTM! Consider organizing test declarations for better maintainability.

The removal of GroupChatIconComponent aligns with the PR changes. The test configuration is correct and follows the coding guidelines.

Consider extracting the declarations into a constant for better maintainability:

const TEST_DECLARATIONS = [
  ConversationAddUsersDialogComponent,
  ConversationAddUsersFormStubComponent,
  MockPipe(ArtemisTranslatePipe),
  MockComponent(ChannelIconComponent),
];

TestBed.configureTestingModule({
  declarations: TEST_DECLARATIONS,
  providers: [/*...*/],
});
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (1)

Line range hint 94-154: Add test coverage for new UI features.

While the existing test cases are comprehensive, consider adding tests for the new UI features:

  1. Verify profile picture rendering in direct chats
  2. Validate group icon display
  3. Test conversation header profile picture visibility

Example test structure:

it('should show profile picture for direct chats', () => {
    const activeConversation = generateOneToOneChatDTO({});
    initializeDialog(component, fixture, { course, activeConversation });
    fixture.detectChanges();
    
    const profilePicture = fixture.debugElement.query(By.css('.profile-picture'));
    expect(profilePicture).toBeTruthy();
});

it('should show group icon for group chats', () => {
    const activeConversation = generateExampleGroupChatDTO({});
    initializeDialog(component, fixture, { course, activeConversation });
    fixture.detectChanges();
    
    const groupIcon = fixture.debugElement.query(By.css('fa-icon'));
    expect(groupIcon).toBeTruthy();
    expect(groupIcon.componentInstance.icon).toBe('faPeopleGroup');
});
src/main/webapp/app/shared/sidebar/sidebar.component.ts (1)

Line range hint 23-29: Add JSDoc comments for improved documentation.

Consider adding JSDoc comments to document the purpose of the new output and input properties, especially since they're part of the public API.

Example:

/** Emits when a direct chat is initiated */
onDirectChatPressed = output<void>();

/** Emits when a group chat is initiated */
onGroupChatPressed = output<void>();

/** Controls whether the component is used in communication context */
inCommunication = input<boolean>(false);
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (1)

30-30: Consider using path aliases for cleaner imports.

The import statement uses a long relative path with multiple parent directory references. Consider configuring TypeScript path aliases in tsconfig.json to make imports more maintainable.

Example configuration:

// tsconfig.json
{
  "compilerOptions": {
    "paths": {
+     "@shared/*": ["app/shared/*"]
    }
  }
}

Then update the import:

-import { ProfilePictureComponent } from '../../../../../../../../main/webapp/app/shared/profile-picture/profile-picture.component';
+import { ProfilePictureComponent } from '@shared/profile-picture/profile-picture.component';
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c9f0c0d and b3b6839.

📒 Files selected for processing (17)
  • src/main/webapp/app/entities/metis/conversation/one-to-one-chat.model.ts (1 hunks)
  • src/main/webapp/app/overview/course-conversations/course-conversations.module.ts (0 hunks)
  • src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html (1 hunks)
  • src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts (2 hunks)
  • src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.html (1 hunks)
  • src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts (6 hunks)
  • src/main/webapp/app/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.html (0 hunks)
  • src/main/webapp/app/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.ts (0 hunks)
  • src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html (1 hunks)
  • src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (3 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/shared/sidebar/sidebar.module.ts (2 hunks)
  • src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/conversation-add-users-dialog.component.spec.ts (1 hunks)
  • src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (2 hunks)
  • src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (3 hunks)
  • src/test/javascript/spec/component/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.spec.ts (0 hunks)
💤 Files with no reviewable changes (4)
  • src/main/webapp/app/overview/course-conversations/course-conversations.module.ts
  • src/main/webapp/app/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.html
  • src/main/webapp/app/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.ts
  • src/test/javascript/spec/component/overview/course-conversations/other/group-chat-icon/group-chat-icon.component.spec.ts
✅ Files skipped from review due to trivial changes (1)
  • src/main/webapp/app/shared/sidebar/sidebar.component.html
🧰 Additional context used
📓 Path-based instructions (12)
src/main/webapp/app/entities/metis/conversation/one-to-one-chat.model.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.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/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.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/layout/conversation-header/conversation-header.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.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-card-item/sidebar-card-item.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/shared/sidebar/sidebar.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/shared/sidebar/sidebar.module.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/conversation-add-users-dialog.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

🔇 Additional comments (18)
src/main/webapp/app/entities/metis/conversation/one-to-one-chat.model.ts (1)

19-24: LGTM! Well-structured type conversion utility.

The function is well-implemented and serves its purpose in supporting the UI differentiation between chat types. It:

  • Handles undefined cases safely
  • Reuses the existing type guard
  • Follows TypeScript best practices
  • Aligns with the PR objective of improving chat type distinction
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (2)

3-4: LGTM: Imports are appropriate and follow guidelines.

The new imports support the chat type differentiation requirements while adhering to the Angular style guide.


24-24: LGTM: Appropriate lifecycle hook usage.

The initialization of user data in ngOnInit follows Angular best practices.

src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts (2)

10-10: LGTM: Import statement follows Angular style guide

The Font Awesome icon import is correctly placed and aligns with the PR objective of improving visual distinction between chat types.


28-28: LGTM: Property declaration follows best practices

The faPeopleGroup property is correctly declared as readonly and follows Angular naming conventions.

src/main/webapp/app/shared/sidebar/sidebar.module.ts (1)

19-19: LGTM! Import statement follows Angular guidelines.

The import statement follows proper conventions with absolute paths and PascalCase naming.

src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html (2)

Line range hint 1-93: Well-structured template with proper Angular syntax usage.

The template demonstrates excellent practices:

  • Consistent usage of new Angular syntax (@if, @switch)
  • Clear separation between different conversation types
  • Proper initialization check
  • Well-organized sections for header, tabs, and content

9-10: Verify icon size consistency across different chat types.

The Font Awesome icon replacement looks good and aligns with the PR's objective of improving visual distinction. However, let's ensure the size="xs" matches the size of other icons (channel icon) in the header for visual consistency.

✅ Verification successful

Icon size is consistent with other similar use cases

The size="xs" attribute for the group chat icon is consistent with other icons used in similar contexts within the conversation components. Specifically:

  • The same size="xs" is used for icons in the conversation member row component
  • The group chat icon in the conversation header also uses size="xs"
  • Larger sizes are only used for special cases like loading spinners (3x) and empty state icons (5x)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if other conversation icons use consistent size attributes
# Expected: All icon size attributes should match for visual consistency

# Search for icon size attributes in conversation-related components
rg -g '*.html' 'size="[^"]*".*(?:fa-icon|channel-icon)'

Length of output: 287


Script:

#!/bin/bash
# Let's search more specifically in the conversation-related components
rg -g '*conversation*.html' '<fa-icon' -A 1 -B 1

# Also check for channel-icon usage
rg -g '*conversation*.html' '<channel-icon' -A 1 -B 1

Length of output: 13365

src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.html (1)

16-18: LGTM! Verify icon consistency across the application.

The group chat icon implementation using Font Awesome is appropriate and follows the new Angular syntax guidelines.

✅ Verification successful

Icon usage is consistent across the application

The faPeopleGroup icon is consistently used across the application for group chat representation:

  • Conversation header component for active conversations
  • Conversation detail dialog for group chat details
  • Sidebar component for group chat creation
  • Sidebar card items for group chat entries
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify consistent usage of group chat icon across the application
rg -l "faPeopleGroup" --type ts

Length of output: 406


Script:

#!/bin/bash
# Check the actual usage of faPeopleGroup icon in the components
rg "faPeopleGroup" -A 2 -B 2 --type ts

Length of output: 6080


Script:

#!/bin/bash
# Check the HTML templates for consistent usage of the group chat icon
rg "faPeopleGroup" --type html -A 2 -B 2

Length of output: 2507

src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts (3)

2-2: LGTM: Import statements are well-organized.

The new imports support the feature requirements for differentiating chat types and handling user profile information.

Also applies to: 21-22


44-44: LGTM: Properties are well-defined and properly typed.

The new properties follow Angular style guidelines and maintain type safety:

  • otherUser for storing the other participant's information in one-to-one chats
  • readonly faPeopleGroup for the group chat icon

Also applies to: 50-50


94-94: LGTM: Subscription handling is correct.

The getOtherUser() call is properly integrated into the existing subscription, and memory leak prevention is maintained through the use of takeUntil.

src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (2)

4-4: LGTM: Import changes align with icon replacement.

The addition of FontAwesomeModule import and removal of GroupChatIconComponent are consistent with the PR's objective to update the icon system.


86-86: LGTM: TestBed configuration properly updated.

The TestBed configuration correctly includes FontAwesomeModule and follows Angular testing best practices.

src/main/webapp/app/shared/sidebar/sidebar.component.ts (3)

2-2: LGTM: Icon change aligns with UI enhancement goals.

The replacement of faUsers with faPeopleGroup provides better visual distinction for group chats, consistent with the PR objectives.


60-60: LGTM: Icon property follows Angular guidelines.

The faPeopleGroup readonly property is properly initialized and follows naming conventions.


Line range hint 33-36: Verify backward compatibility of input/output changes.

The changes to use modern Angular syntax (output<void>() and required inputs) improve type safety but may affect existing code.

Also applies to: 45-45, 52-52

✅ Verification successful

Based on the search results, I can now generate a final response as I have found all the component usages and can verify the impact of the changes.

Input/output changes are safe and properly handled in all usages.

The component usages show that:

  • All instances provide the required inputs properly
  • The new channelTypeIcon property is only used in the communication context where it's explicitly provided
  • The itemSelected boolean is correctly used across different contexts (exercises, lectures, exams, etc.)
  • The courseId input is consistently provided in all parent components
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing usages of the component to ensure they provide required inputs
ast-grep --pattern 'jhi-sidebar $$$'

Length of output: 162


Script:

#!/bin/bash
# Search for component usages with ripgrep for better encoding handling
rg -l "<jhi-sidebar" --type html

# Then get content of any found files
for file in $(rg -l "<jhi-sidebar" --type html); do
    echo "=== $file ==="
    cat "$file"
done

Length of output: 21576

src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (1)

54-54: LGTM!

The test module setup correctly uses NgMocks to mock the ProfilePictureComponent, following the coding guidelines.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (1)

40-50: Improve component architecture and method organization.

The current implementation has several architectural concerns:

  1. The method is marked as private in the code but implemented as public
  2. It violates Single Responsibility Principle by handling both user extraction and icon assignment
  3. The icon assignment could be more reactive

Consider splitting the responsibilities:

-    extractMessageUser(): void {
+    private extractMessageUser(): void {
         if (this.sidebarItem.type === 'oneToOneChat' && (this.sidebarItem.conversation as OneToOneChatDTO)?.members) {
             this.otherUser = (this.sidebarItem.conversation as OneToOneChatDTO).members!.find((user) => !user.isRequestingUser);
         } else {
             this.otherUser = null;
         }
+    }
 
+    get sidebarIcon(): any {
         if (this.sidebarItem.type === 'groupChat') {
-            this.sidebarItem.icon = this.faPeopleGroup;
+            return this.faPeopleGroup;
         }
-    }
+        return this.sidebarItem.icon;
+    }

Then update the template to use:

[icon]="sidebarIcon"
src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts (3)

26-38: Consider using TypeScript interface for mock data.

While the mock data is well-structured, consider defining a proper interface instead of using any type for better type safety and maintainability.

interface SidebarItemMock {
    title: string;
    id: string;
    size: SidebarCardSize;
    difficulty: DifficultyLevel;
    type: string;
    conversation: OneToOneChatDTO;
}

69-74: Improve group chat icon test robustness.

The test could be more robust with a proper setup and specific assertions.

 it('should set group icon for group chats in extractMessageUser', () => {
-    component.sidebarItem.type = 'groupChat';
-    component.sidebarItem.icon = undefined;
+    component.sidebarItem = {
+        ...sidebarItemMock,
+        type: 'groupChat',
+    };
     component.extractMessageUser();
-    expect(component.sidebarItem.icon).toBe(faPeopleGroup);
+    expect(component.sidebarItem.icon).toEqual(faPeopleGroup);
 });

76-79: Enhance one-to-one chat test reliability.

The test could be more reliable with explicit member selection and specific assertions.

 it('should set otherUser for one-to-one chat in extractMessageUser', () => {
+    const expectedOtherUser = sidebarItemMock.conversation.members.find(m => !m.isRequestingUser);
     component.extractMessageUser();
-    expect(component.otherUser).toEqual(sidebarItemMock.conversation.members[1]);
+    expect(component.otherUser).toEqual(expectedOtherUser);
+    expect(component.otherUser.name).toBe('User1');
+    expect(component.otherUser.isRequestingUser).toBeFalse();
 });
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b3b6839 and 2e5ecff.

📒 Files selected for processing (2)
  • src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (3 hunks)
  • src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

🔇 Additional comments (4)
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (1)

3-4: LGTM: Clean imports and icon declaration.

The imports are well-organized and the FontAwesome icon is correctly declared as readonly.

Also applies to: 18-18

src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts (3)

6-10: LGTM! Imports follow best practices.

The new imports are properly organized and the use of MockComponent from ng-mocks aligns with our testing guidelines.


19-21: LGTM! Proper component mocking setup.

The TestBed configuration correctly uses MockComponent for ProfilePictureComponent as per our testing guidelines.


49-51: LGTM! Proper test assertion.

The test uses specific assertion (toContain) as per our testing guidelines.

Copy link
Contributor

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS1 and works as expected. Nice change!

Copy link

@julian-wls julian-wls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested it on TS1. Works as expected.
Maybe one small remark:

I noticed, that the space between the image and the title is quite small:
Screenshot3

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS1. Works as described

Screenshot 2024-11-09 162343
Screenshot 2024-11-09 162350

Copy link

@sachmii sachmii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS1, works as described.

Copy link
Contributor

@PaRangger PaRangger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM. Tested on TS1, looks great 👍

Copy link

@ahbitaqu ahbitaqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Tested on TS2]
Works as described! Couldn't find any issues
image
image

@krusche krusche added this to the 7.7.0 milestone Nov 10, 2024
@krusche krusche merged commit cec3866 into develop Nov 10, 2024
82 of 86 checks passed
@krusche krusche deleted the feature/communication/add-profile-picture-to-sidebar branch November 10, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) component:Communication ready to merge tests
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

8 participants