-
Notifications
You must be signed in to change notification settings - Fork 294
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
: Improve unread messages count view on sidebar
#9522
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🔇 Additional comments (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
|
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: 4
🧹 Outside diff range and nitpick comments (8)
src/main/webapp/app/shared/sidebar/sidebar-card-large/sidebar-card-large.component.html (1)
8-8
: LGTM! Consider a minor improvement.The addition of the
unreadCount
input to thejhi-sidebar-card-item
component aligns well with the PR objectives. The use of optional chaining forconversation?.unreadMessagesCount
is a good practice to prevent potential errors.Consider removing the
this
keyword from the template expression. In Angular templates,this
is implicit and unnecessary. Here's the suggested change:- <jhi-sidebar-card-item [unreadCount]="this.sidebarItem.conversation?.unreadMessagesCount" [sidebarType]="sidebarType" [sidebarItem]="sidebarItem" /> + <jhi-sidebar-card-item [unreadCount]="sidebarItem.conversation?.unreadMessagesCount" [sidebarType]="sidebarType" [sidebarItem]="sidebarItem" />This change would make the code slightly cleaner and more idiomatic Angular.
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.scss (1)
9-23
: LGTM! Well-implemented unread count badge.The
.unread-count
class is well-designed for displaying unread message counts. It uses appropriate styling for visibility, responsiveness, and layout. The use of CSS variables and theclamp()
function for font sizing are particularly commendable.Consider adding a
min-width
property to ensure the badge maintains its circular shape even with single-digit numbers:.unread-count { /* ... existing properties ... */ + min-width: 1.5rem; }
This will ensure consistency in the badge's appearance across different count values.
src/main/webapp/app/shared/sidebar/sidebar-card-small/sidebar-card-small.component.html (2)
Line range hint
1-2
: Consider adding more specific information about the follow-up PR.The comment provides good context for the temporary workaround. To improve traceability, consider adding a reference to a specific issue or ticket number for the follow-up PR that will address this workaround.
Line range hint
17-21
: Update to new Angular control flow syntax.As per the coding guidelines, please update the
*ngIf
directive to the new@if
syntax. This change will align the code with the latest Angular best practices and the project's coding standards.Apply this change:
- @if (sidebarItem.conversation) { - <div> - <jhi-conversation-options [conversation]="sidebarItem.conversation!" (onUpdateSidebar)="onUpdateSidebar.emit()" /> - </div> - } + @if (sidebarItem.conversation) { + <div> + <jhi-conversation-options [conversation]="sidebarItem.conversation!" (onUpdateSidebar)="onUpdateSidebar.emit()" /> + </div> + }src/main/webapp/app/shared/sidebar/sidebar-card-medium/sidebar-card-medium.component.html (1)
Line range hint
10-24
: LGTM with a minor suggestion: Non-exam mode section is correctly updatedThe non-exam mode section has been correctly updated to include the
unreadCount
property in thejhi-sidebar-card-item
component, which aligns with the PR objectives to display unread message counts. The use of @else also adheres to the new Angular syntax guidelines.Consider removing
this.
from the template expression:- <jhi-sidebar-card-item [unreadCount]="this.sidebarItem.conversation?.unreadMessagesCount" [sidebarType]="sidebarType" [sidebarItem]="sidebarItem" /> + <jhi-sidebar-card-item [unreadCount]="sidebarItem.conversation?.unreadMessagesCount" [sidebarType]="sidebarType" [sidebarItem]="sidebarItem" />This change improves readability and follows Angular best practices for template expressions.
src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts (2)
53-57
: LGTM with a minor suggestion: Consider testing initial state.This test case effectively verifies the behavior of
formattedUnreadCount
whenunreadCount
is undefined. It follows the jest syntax and uses appropriate expectations.However, consider testing the initial state of the component before calling
ngOnInit()
. This would ensure that theformattedUnreadCount
is correctly initialized.Here's a suggested improvement:
it('should return an empty string if unreadCount is undefined', () => { component.unreadCount = undefined; expect(component.formattedUnreadCount).toBe(''); // Test initial state component.ngOnInit(); expect(component.formattedUnreadCount).toBe(''); // Test after initialization });
Line range hint
1-57
: Consider enhancing test coverage with additional scenarios.The new test cases significantly improve the coverage for the
formattedUnreadCount
property. To further enhance the test suite, consider the following suggestions:
- Test edge cases: Add tests for unreadCount values of 0, 1, 98, 99, and 100 to ensure correct behavior at boundary conditions.
- Test reactivity: Verify that changes to
unreadCount
after initialization are reflected informattedUnreadCount
.- Test integration: Add a test to ensure the formatted count is correctly displayed in the component's template.
Here's an example of how you might implement the first suggestion:
it('should handle edge cases for unreadCount', () => { const testCases = [ { input: 0, expected: '0' }, { input: 1, expected: '1' }, { input: 98, expected: '98' }, { input: 99, expected: '99' }, { input: 100, expected: '99+' }, ]; testCases.forEach(({ input, expected }) => { component.unreadCount = input; component.ngOnInit(); expect(component.formattedUnreadCount).toBe(expected); }); });src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html (1)
Line range hint
63-63
: Simplify the condition in@if
statementIn line 63, the condition
@if (unreadCount && unreadCount > 0)
can be simplified by removing the redundantunreadCount
check. SinceunreadCount > 0
effectively handles the desired condition, you can simplify it to:@if (unreadCount > 0) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html (2 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.scss (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-card-large/sidebar-card-large.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-card-medium/sidebar-card-medium.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-card-small/sidebar-card-small.component.html (1 hunks)
- src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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)
src/main/webapp/app/shared/sidebar/sidebar-card-large/sidebar-card-large.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-medium/sidebar-card-medium.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-small/sidebar-card-small.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/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 (12)
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.scss (2)
Line range hint
1-27
: Changes align well with PR objectives and enhance UI functionality.The addition of the
.unread-count
and.bold-text
classes effectively supports the PR's goal of enhancing the visibility of unread messages in the sidebar. These styles provide the necessary visual cues to draw users' attention to conversations with unread messages.The implementation appears to meet the requirements outlined in the PR objectives. However, to ensure full compliance with the design specifications:
- Can you confirm that the
.unread-count
class handles counts exceeding 99 with the special format (99+) as mentioned in the PR objectives?- Are there any accessibility considerations (e.g., color contrast) that need to be addressed for the unread count badge?
#!/bin/bash # Check for handling of large unread counts and accessibility considerations rg --type typescript "unreadCount" src/ rg --type html "unreadCount" src/ rg --type scss "color contrast|a11y|accessibility" src/This will help us verify the implementation of the count formatting and any existing accessibility measures.
25-27
: Consider using a more standard font-weight value.The
.bold-text
class uses a font-weight of 850, which is unusually high. Standard bold typically uses a value of 700, and many fonts don't support weights above 900.Consider adjusting the font-weight to a more standard value:
.bold-text { - font-weight: 850; + font-weight: 700; // or 800 if extra emphasis is needed }Could you clarify the design decision behind using such a high font-weight? If this level of emphasis is crucial for the UI, we should ensure it renders consistently across different system fonts and browsers.
This will help us understand if this is a one-off decision or part of a broader design pattern.
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (4)
1-1
: LGTM: Import statement updated correctly.The addition of
OnInit
to the import statement is appropriate given the class now implements this interface. The use of single quotes for the import path adheres to the coding guidelines.
9-9
: LGTM: Class correctly implements OnInit.The
SidebarCardItemComponent
now implementsOnInit
, which is a good practice for handling initialization logic. The class name follows PascalCase, adhering to the coding guidelines.
16-18
: LGTM: ngOnInit implemented correctly.The
ngOnInit
method is correctly implemented, initializing theformattedUnreadCount
property. This follows Angular lifecycle hooks best practices and the method name adheres to camelCase convention as per guidelines.
1-25
: Overall, the changes improve the component's functionality and mostly adhere to guidelines.The
SidebarCardItemComponent
has been successfully updated to handle and display unread message counts. The changes generally follow Angular best practices and project-specific guidelines. A few suggestions have been made to further improve the code:
- Consider a safer approach for the
unreadCount
input property to prevent potential runtime errors.- The
getFormattedUnreadCount
method could be made more concise.These minor adjustments would enhance the robustness and readability of the component. Great job on implementing this new feature!
src/main/webapp/app/shared/sidebar/sidebar-card-small/sidebar-card-small.component.html (2)
Line range hint
8-9
: Verify relative routing paths.The template uses relative paths for routing (
'../communication'
and'./communication'
). While this might be correct, it's important to ensure these paths resolve correctly in all scenarios.Please run the following verification script to check the routing configuration:
#!/bin/bash # Description: Verify the routing configuration for the communication module # Test: Check the routing configuration in the module file rg --type typescript -A 10 "RouterModule.forChild" src/main/webapp/app/communication # Test: Verify the parent route configuration rg --type typescript -A 10 "{ path: 'communication'" src/main/webapp/app/layouts
13-13
: LGTM! Verify unread count implementation.The addition of the
unreadCount
property to thejhi-sidebar-card-item
component is well-implemented and aligns with the PR objectives. The use of optional chaining forthis.sidebarItem.conversation?.unreadMessagesCount
is a good practice for safe property access.To ensure the
unreadCount
property is correctly implemented in thesidebar-card-item
component, please run the following verification script:src/main/webapp/app/shared/sidebar/sidebar-card-medium/sidebar-card-medium.component.html (1)
Line range hint
1-9
: LGTM: Exam mode section is correctly implementedThe 'inExam' mode section is correctly implemented using the new Angular syntax @if, which aligns with the provided coding guidelines. The structure and functionality remain unchanged and appropriate for exam mode.
src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts (3)
41-45
: LGTM: Well-structured test case for unreadCount formatting.This test case effectively verifies the behavior of
formattedUnreadCount
for values less than 99. It follows the jest syntax and uses appropriate expectations.
47-51
: LGTM: Effective test for unreadCount exceeding 99.This test case properly verifies the behavior of
formattedUnreadCount
for values greater than 99. It adheres to the jest syntax and uses appropriate expectations.
Line range hint
1-57
: Overall, good additions to the test suite with room for further enhancements.The new test cases for
formattedUnreadCount
are well-structured and improve the overall coverage of theSidebarCardItemComponent
. They follow jest syntax and adhere to the provided coding guidelines. The suggestions provided in the previous comments, if implemented, would further strengthen the test suite and ensure more comprehensive coverage of the component's behavior.
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html
Outdated
Show resolved
Hide resolved
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.
I have some remarks.
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-large/sidebar-card-large.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-medium/sidebar-card-medium.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-small/sidebar-card-small.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.scss
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.scss
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html
Outdated
Show resolved
Hide resolved
# Conflicts: # src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts # src/main/webapp/app/shared/sidebar/sidebar-card-large/sidebar-card-large.component.html # src/main/webapp/app/shared/sidebar/sidebar-card-medium/sidebar-card-medium.component.html # src/main/webapp/app/shared/sidebar/sidebar-card-small/sidebar-card-small.component.html
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts
Show resolved
Hide resolved
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: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (2)
17-25
: LGTM: Lifecycle hooks correctly implemented with a minor optimization suggestion.The ngOnInit and ngOnChanges methods are correctly implemented to initialize and update the formattedUnreadCount. This addresses the previous concern about reacting to changes in unreadCount. The implementation follows Angular lifecycle hook best practices and adheres to the coding guidelines for indentation and bracing style.
Consider optimizing the ngOnChanges method to avoid unnecessary updates:
ngOnChanges(changes: SimpleChanges): void { if (changes['unreadCount'] && !changes['unreadCount'].firstChange) { this.formattedUnreadCount = this.getFormattedUnreadCount(); } }This ensures that the formatted count is only updated when the unreadCount changes after the initial rendering.
27-32
: LGTM: Formatting method correctly implemented with a minor readability suggestion.The
getFormattedUnreadCount
method correctly formats the unread count as per requirements. It properly uses the function call syntax for the input signalunreadCount()
. The implementation is concise and follows the coding guidelines.Consider using a ternary operator for improved readability:
private getFormattedUnreadCount(): string { const count = this.unreadCount(); return count > 99 ? '99+' : count.toString(); }This maintains the same logic while slightly improving readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html (2 hunks)
- src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (1 hunks)
- src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html
- src/test/javascript/spec/component/shared/sidebar/sidebar-card-item.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (1)
🔇 Additional comments (4)
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.ts (4)
1-1
: LGTM: Imports updated correctly.The new imports (OnChanges, OnInit, SimpleChanges, input) are correctly added to support the implementation of lifecycle hooks and input signals. This aligns with Angular best practices and the suggestion to use input signals.
9-9
: LGTM: Lifecycle interfaces correctly implemented.The class now implements OnInit and OnChanges interfaces, which is necessary for the lifecycle hooks implemented in the component. This follows Angular best practices for component lifecycle management.
13-13
: LGTM: Input property correctly implemented using signals.The
unreadCount
input property is correctly implemented using input signals with a default value of 0. This addresses previous suggestions and follows the coding guidelines:
- Uses input signals as recommended in Angular documentation.
- Provides a default value to prevent potential runtime errors.
- Follows the camelCase naming convention for properties.
1-32
: Overall, the changes look good and address the PR objectives.The implementation of the unread count display in the sidebar card item is well done. It addresses previous comments, follows Angular best practices, and adheres to the coding guidelines. The use of input signals, proper lifecycle management, and correct formatting of the unread count contribute to a robust and maintainable component.
A few minor suggestions for optimization and readability have been provided, but these are not critical and can be addressed at the developer's discretion.
Great job on implementing this feature!
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.
Works on Ts4, when adding new messages, the number of new messages shows up, nice improvement 👍
Two remarks:
- should the number disappear after I clicked on the channel? I think at them moment there is some kind of timer?
- also, when I add messages myself, should I see the new message count go up?
What would also be nice in a follow up, is to directly see new incoming messages, without needing to click somewhere to update the channels :D
I also noticed the second point made by @SimonEntholzer . This only happens tho if the chat window is not fully at the bottom. If one scrolls fully down and then writes a message the counter does not go up. I think if we change the behaviour of the chat window opening to the latest message always then this should not be an issue anymore. |
Communication
: Refactor unread messages count view on sidebarCommunication
: Improve unread messages count view on sidebar
(cherry picked from commit 6e64f08)
Checklist
General
Client
Motivation and Context
Previously, the sidebar did not display the unread message count for the relevant conversations. This update refactors the view to show the number of unread messages next to each conversation in the sidebar.
Description
The unread count is now displayed next to the conversation title, with a special format (99+) for counts exceeding 99. Additionally, the conversation title is now displayed in bold when there are unread messages, enhancing the visibility of conversations that require attention.
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
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Style