Communication: Fix clipping issues on mobile devices
#9460
Conversation
github-actions
bot
added
tests
client
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces several modifications to the Angular application, primarily focusing on enhancing the sidebar functionality based on device type. Key changes include the addition of mobile detection logic, the implementation of a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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.
Caution
Inline review comments failed to post
Actionable comments posted: 31
🧹 Outside diff range comments (13)
src/main/webapp/app/shared/metis/metis.component.scss (1)
Line range hint
7-126: Existing styles maintained with improved interactivity.The existing styles have been preserved, maintaining consistency with the current design. The addition of a hover effect to
.post-edit-profile-picture-overlayenhances user interaction.Consider adding a
transitionproperty to.post-edit-profile-picture-overlayfor a smoother hover effect:.post-edit-profile-picture-overlay { position: absolute; top: 0; left: 0; width: 100%; height: 100%; background-color: rgba(#000, 0.6); opacity: 0; color: var(--white); + transition: opacity 0.3s ease; }🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 128-128: src/main/webapp/app/shared/metis/metis.component.scss#L128
Unexpected unknown at-rule "@include" (at-rule-no-unknown)src/main/webapp/app/overview/course-overview.component.scss (4)
Line range hint
15-131: LGTM! Enhanced responsiveness and user experience.The updated styles for the sidebar and drawer container significantly improve responsiveness across different screen sizes and device types. The use of CSS variables and transitions enhances maintainability and user experience.
For consistency, consider using CSS variables for all fixed dimensions, such as
$menu-width-closedand$menu-width-open.
Line range hint
133-226: LGTM! Improved visual feedback and hierarchy.The updated styles for navigation links, message indicators, and course circles enhance the visual feedback and hierarchy of the UI elements. The use of placeholder selectors for message blocks is a good practice for code reuse.
Consider adding appropriate
aria-labelattributes to elements with visual-only indicators (like the new message dot) to improve accessibility for screen readers.
Line range hint
228-358: LGTM! Enhanced interactivity and visual feedback.The new styles for the sidebar collapse button and dropdown menu significantly improve the interactivity and visual feedback of the UI. The use of transitions and pseudo-elements creates a smooth and engaging user experience.
To potentially improve performance, consider using
transforminstead ofwidthfor animations where possible, as it's generally more efficient for the browser to render.
Line range hint
360-408: LGTM! Improved print layout and mobile-specific adjustments.The addition of print styles and the mobile-specific media query for the communication module enhances the overall user experience across different contexts. The print styles effectively hide unnecessary elements, while the mobile adjustments improve usability on small screens.
For consistency, consider using the same breakpoint variable or mixin for all mobile-specific styles throughout the file, instead of mixing
max-width: 768pxand@include media-breakpoint-down(sm).src/main/webapp/app/overview/course-overview.component.ts (2)
Line range hint
70-102: LGTM! Consider using a consistent naming pattern for subscriptions.The addition of the CourseSidebarService import and the new subscriptions for managing sidebar events are well-implemented and align with the PR objectives. The naming convention follows the Angular style guide, and the subscriptions are properly typed.
For improved consistency, consider using a common prefix for all sidebar-related subscriptions:
-private closeSidebarEventSubscription: Subscription; -private openSidebarEventSubscription: Subscription; -private toggleSidebarEventSubscription: Subscription; +private sidebarCloseSubscription: Subscription; +private sidebarOpenSubscription: Subscription; +private sidebarToggleSubscription: Subscription;This minor change would make the code more uniform and easier to read.
Line range hint
786-790: LGTM! Consider adding a comment for clarity.The implementation of the keyboard shortcut for toggling the sidebar is well done and improves accessibility and user experience. The use of @HostListener is appropriate, and preventing the default behavior ensures no unintended browser actions occur.
To improve code clarity, consider adding a brief comment explaining the purpose of this shortcut:
/** * Toggles the sidebar visibility when the user presses Ctrl+Shift+B. * This keyboard shortcut improves accessibility and provides a quick way to show/hide the sidebar. */ @HostListener('window:keydown.Control.Shift.b', ['$event']) onKeyDownControlShiftB(event: KeyboardEvent) { // ... existing code ... }This comment will help other developers understand the purpose and importance of this keyboard shortcut.
src/test/javascript/spec/component/course/course-overview.component.spec.ts (6)
Line range hint
290-297: LGTM! Consider enhancing test readability.The test case is well-structured and effectively verifies the creation of sidebar items when the student course analytics dashboard feature is active. It correctly checks for the presence and order of expected items.
To improve readability, consider using constants for the expected sidebar item titles:
const EXPECTED_TITLES = ['Dashboard', 'Exercises', 'Lectures']; sidebarItems.forEach((item, index) => { expect(item.title).toContain(EXPECTED_TITLES[index]); });
Line range hint
300-306: LGTM! Consider consistent testing approach.The test case effectively verifies the creation of default sidebar items when the student course analytics dashboard feature is not active. It correctly checks for the presence and order of expected items.
For consistency with the previous test and to improve maintainability, consider using a similar approach with constants:
const EXPECTED_TITLES = ['Exercises', 'Lectures']; sidebarItems.forEach((item, index) => { expect(item.title).toContain(EXPECTED_TITLES[index]); });
Line range hint
351-367: LGTM! Consider improving test isolation.The test case effectively verifies that message-related data is not loaded when messaging is disabled for a course. It correctly checks that specific methods are not called when navigating through different tabs.
To improve test isolation and readability, consider extracting the tab navigation logic into a separate helper method:
function navigateThroughTabs(tabs: string[]) { tabs.forEach((tab) => { route.snapshot.firstChild!.routeConfig!.path = tab; component.onSubRouteActivate({ controlConfiguration: undefined }); }); } // In the test navigateThroughTabs(['exercises', 'communication', 'exercises', 'communication']);This approach would make the test more maintainable and easier to understand.
Line range hint
409-425: LGTM! Consider enhancing error handling specificity.The test case effectively verifies the error handling behavior when loading the course fails. It correctly checks that an alert is shown in response to a 404 error.
To improve the test's robustness and specificity, consider checking the content of the alert message:
const alertServiceSpy = jest.spyOn(alertService, 'addAlert'); component.loadCourse().subscribe( () => { throw new Error('should not happen'); }, (error) => { expect(error).toBeDefined(); expect(alertServiceSpy).toHaveBeenCalledWith({ type: 'danger', msg: 'artemisApp.course.errors.notFound', }); }, );This ensures that not only is an alert shown, but it's the correct alert for the specific error scenario.
Line range hint
427-436: LGTM! Consider enhancing test clarity.The test case effectively verifies the behavior of canRegisterForCourse when the server returns a 403 error. It correctly uses fakeAsync and tick for handling asynchronous behavior.
To improve the test's clarity and robustness, consider using the
toBeObservablematcher from RxJS testing utilities:import { TestScheduler } from 'rxjs/testing'; it('should return false for canRegisterForCourse if the server returns 403', () => { const scheduler = new TestScheduler((actual, expected) => { expect(actual).toEqual(expected); }); scheduler.run(({ expectObservable }) => { findOneForRegistrationStub.mockReturnValue(throwError(() => new HttpResponse({ status: 403 }))); const expected = '(a|)'; const expectedValues = { a: false }; expectObservable(component.canRegisterForCourse()).toBe(expected, expectedValues); }); });This approach provides a more declarative way of testing observables and eliminates the need for fakeAsync and tick.
Line range hint
622-628: LGTM! Consider enhancing test completeness.The test case effectively verifies the basic functionality of the toggleCollapseState method. It correctly checks that the isNavbarCollapsed property is toggled as expected.
To improve the test's completeness, consider adding an initial state check and verifying the method's behavior when called an odd number of times:
it('should toggle isNavbarCollapsed when toggleCollapseState is called', () => { fixture.detectChanges(); // Check initial state expect(component.isNavbarCollapsed).toBeFalse(); component.toggleCollapseState(); expect(component.isNavbarCollapsed).toBeTrue(); component.toggleCollapseState(); expect(component.isNavbarCollapsed).toBeFalse(); // Check behavior when called an odd number of times component.toggleCollapseState(); expect(component.isNavbarCollapsed).toBeTrue(); });This ensures that the method behaves correctly regardless of the number of times it's called and verifies the initial state.
🛑 Comments failed to post (31)
src/main/webapp/app/overview/course-sidebar.service.ts (2)
1-24: 🧹 Nitpick (assertive)
Overall implementation is solid, consider adding JSDoc comments.
The
CourseSidebarServiceis well-implemented, following Angular best practices and the provided coding guidelines. It provides a clean API for managing sidebar visibility using EventEmitters, which allows for loose coupling between components.To further improve the code:
Consider adding JSDoc comments to the class and its methods to enhance code documentation. This will provide better context for other developers using this service. For example:
/** * Service for managing the visibility of the course sidebar. */ export class CourseSidebarService { // ... existing properties ... /** * Emits an event to close the sidebar. */ public closeSidebar(): void { // ... existing implementation ... } // ... add similar comments for other methods ... }🧰 Tools
🪛 Biome
[error] 11-11: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
11-11:
⚠️ Potential issueRemove unnecessary constructor.
The empty constructor doesn't serve any purpose and can be safely removed to improve code clarity.
Apply this diff to remove the unnecessary constructor:
- constructor() {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.🧰 Tools
🪛 Biome
[error] 11-11: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.scss (1)
43-43: 💡 Codebase verification
Multiple SCSS files utilize
vhunits, which may affect the effectiveness of themax-heightadjustment in addressing mobile clipping issues.
- Consider reviewing and adjusting other
vh-based calculations to ensure consistent behavior across the application.- Implement targeted media queries for mobile devices to more effectively resolve clipping concerns.
🔗 Analysis chain
LGTM, but please clarify the impact of these changes.
The reduction of
max-heightfrom 75vh to 74vh for both production and development environments is noted. While this change aligns with the goal of addressing clipping issues, it's a relatively small adjustment.Could you provide more context on how this 1vh reduction specifically addresses the clipping issues on mobile devices? Are there any test results or visual comparisons that demonstrate the improvement?
Also applies to: 47-47
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other vh-based calculations in the codebase that might need similar adjustments # Test: Search for vh-based calculations in SCSS files rg --type scss '\d+vh' -g '!*conversation-messages.component.scss'Length of output: 97
Script:
#!/bin/bash # Description: Check for other vh-based calculations in SCSS files that might need similar adjustments # Test: Search for vh-based calculations in SCSS files excluding conversation-messages.component.scss rg '\d+vh' -g '*.scss' -g '!.*/conversation-messages.component.scss'Length of output: 13829
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.scss (1)
40-40: 🧹 Nitpick (assertive)
Minor: Consider addressing linter warnings.
Two linter warnings were reported:
- Unknown at-rule "@include" (line 40): This is a false positive as "@include" is a valid SCSS directive.
- Unexpected empty line before declaration (line 58): This is a minor style issue.
While these don't affect functionality, you might consider:
- Configuring the linter to recognize SCSS-specific rules like "@include".
- Removing the empty line before the transition property on line 58 for consistency.
Also applies to: 58-58
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 40-40: src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.scss#L40
Unexpected unknown at-rule "@include" (at-rule-no-unknown)src/test/javascript/spec/service/overview/course-sidebar.service.spec.ts (4)
13-15: 🧹 Nitpick (assertive)
Consider using a more specific assertion
While
toBeTruthy()works, it might be more precise to usetoBeDefined()ortoBeInstanceOf(CourseSidebarService)to explicitly check that the service is created and is of the correct type.You could update the test like this:
it('should be created', () => { expect(service).toBeDefined(); expect(service).toBeInstanceOf(CourseSidebarService); });
17-33: 🧹 Nitpick (assertive)
LGTM: Event emission tests are well-structured
The tests for closeSidebar, openSidebar, and toggleSidebar events are correctly implemented using jest.spyOn to monitor event emissions.
Consider using
toHaveBeenCalledTimes(1)instead oftoHaveBeenCalled()to ensure that the emit method is called exactly once. For example:it('should emit closeSidebar event', () => { const emitSpy = jest.spyOn(service.closeSidebar$, 'emit'); service.closeSidebar(); expect(emitSpy).toHaveBeenCalledTimes(1); });This change would make the tests more precise and catch potential issues where an event might be emitted multiple times unintentionally.
35-51: 🧹 Nitpick (assertive)
LGTM: Comprehensive test for event subscriptions
This test case effectively verifies that subscribed callbacks are invoked when events are emitted through the service methods.
Consider the following improvements for increased precision and readability:
- Use
toHaveBeenCalledTimes(1)instead oftoHaveBeenCalled()to ensure each callback is called exactly once.- Group the service method calls and expectations together for better readability.
- Add assertions to check that other spies are not called when they shouldn't be.
Here's an example of how you could refactor the test:
it('should emit events when subscribing', () => { const closeSpy = jest.fn(); const openSpy = jest.fn(); const toggleSpy = jest.fn(); service.closeSidebar$.subscribe(closeSpy); service.openSidebar$.subscribe(openSpy); service.toggleSidebar$.subscribe(toggleSpy); service.closeSidebar(); expect(closeSpy).toHaveBeenCalledTimes(1); expect(openSpy).not.toHaveBeenCalled(); expect(toggleSpy).not.toHaveBeenCalled(); service.openSidebar(); expect(openSpy).toHaveBeenCalledTimes(1); expect(closeSpy).toHaveBeenCalledTimes(1); expect(toggleSpy).not.toHaveBeenCalled(); service.toggleSidebar(); expect(toggleSpy).toHaveBeenCalledTimes(1); expect(closeSpy).toHaveBeenCalledTimes(1); expect(openSpy).toHaveBeenCalledTimes(1); });This refactored version provides more precise assertions and clearly separates the testing of each service method.
1-52: 🧹 Nitpick (assertive)
LGTM: Comprehensive test suite with good structure
The test suite provides good coverage of the CourseSidebarService, testing its creation, all public methods, and both event emission and subscription. The structure is clear and follows good testing practices.
Consider adding the following tests to further improve coverage:
- Test that calling the same method multiple times emits the event each time.
- Verify that unsubscribing from an event prevents the callback from being called.
- Test any error scenarios or edge cases that might exist in the service implementation.
Example of an additional test:
it('should not call callback after unsubscribing', () => { const spy = jest.fn(); const subscription = service.closeSidebar$.subscribe(spy); service.closeSidebar(); expect(spy).toHaveBeenCalledTimes(1); subscription.unsubscribe(); service.closeSidebar(); expect(spy).toHaveBeenCalledTimes(1); // Still only called once });These additional tests would provide even more confidence in the robustness of the service.
src/main/webapp/app/overview/course-conversations/course-conversations.component.scss (3)
43-50: 🧹 Nitpick (assertive)
LGTM: New classes for message wrapping
The addition of
.communication-message-wrapand.communication-answer-message-wrapclasses is a good approach to manage message layout and prevent clipping issues. The use ofmin-widthandmax-widthwill help maintain readability across different screen sizes.Consider adding a comment explaining the purpose of these classes for better maintainability.
94-101: 🧹 Nitpick (assertive)
LGTM: Responsive styling for answer threads
The styling for
.communication-answer-message-wrapeffectively utilizes the available space on mobile devices when an answer thread is open. Settingmax-width: 100%ensures that the content uses the full width of the screen, which is crucial for readability on smaller devices.Consider adding a transition to the display property change for a smoother user experience when opening and closing answer threads. You can use the
transitionproperty withopacityinstead ofdisplayfor a smoother effect.
85-87: 🧹 Nitpick (assertive)
Consider reordering selectors for better specificity management
The static analysis tool flagged a potential issue with selector order. While this doesn't affect functionality, reordering selectors to follow a more specific-to-general pattern can improve code maintainability and reduce the risk of unexpected style overrides.
Consider moving the
.sidebar-wrapselector before more specific selectors like.communication-content-sidebar.sidebar-collapsed .sidebar-wrapif possible, without breaking the intended styling.🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 85-85: src/main/webapp/app/overview/course-conversations/course-conversations.component.scss#L85
Expected selector ".sidebar-wrap" to come before selector ".communication-content-sidebar.sidebar-collapsed .sidebar-wrap" (no-descending-specificity)src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.html (2)
4-8: 🧹 Nitpick (assertive)
Approve mobile sidebar button with accessibility suggestion.
The addition of a mobile-specific button for opening the sidebar is a good improvement for mobile usability. The conditional display and placement are appropriate.
Consider adding an
aria-labelto the button for better accessibility:- <button class="btn btn-sm btn-outline-secondary d-inline-block d-sm-none me-2" (click)="openSidebar()"> + <button class="btn btn-sm btn-outline-secondary d-inline-block d-sm-none me-2" (click)="openSidebar()" aria-label="Open sidebar">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div class="d-flex align-items-center px-3"> <button class="btn btn-sm btn-outline-secondary d-inline-block d-sm-none me-2" (click)="openSidebar()" aria-label="Open sidebar"> <fa-icon [icon]="faChevronLeft" /> </button> <h4 class="pointer d-inline-block rounded py-2 info mb-0" (click)="openConversationDetailDialog($event, INFO)">
44-44: 💡 Codebase verification
Inconsistent margin classes in button groups.
The
me-1class has been removed from thebtn-groupdiv inconversation-header.component.html. However, other components still use theme-1class withbtn-group, which may lead to inconsistent spacing across the application.Please clarify the reason for this change. If it's intentional, consider applying it uniformly or updating the design guidelines to ensure consistency.
🔗 Analysis chain
Clarify the removal of margin class from button group.
The
me-1class has been removed from the button group div. While this might be intentional to allow the parent container's gap property to control spacing, it's not clear if this change is necessary or if it might cause inconsistencies with other similar elements.Could you please clarify the reasoning behind removing this margin class? If it's intentional, consider adding a comment explaining the decision to maintain consistency across the codebase.
To verify the impact of this change, you can run the following command to check for other occurrences of similar button groups:
This will help ensure that the spacing remains consistent across all button groups in the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar button group structures in HTML files rg --type html 'class="btn-group.*?role="group"' -A 5 -B 5Length of output: 41952
src/main/webapp/app/overview/course-conversations/course-conversations.component.html (3)
43-46: 🧹 Nitpick (assertive)
LGTM: Improved semantic structure and conditional styling
The updates to the main content area's class structure enhance the semantic clarity and allow for more dynamic styling based on the environment and thread state. The use of
communication-message-wrapas the main class name is descriptive and follows good naming conventions.Consider simplifying the
ngClassbinding for better readability:- [ngClass]="{ 'content-height-dev': !isProduction || isTestServer, 'is-answer-thread-open': !!postInThread }" + [ngClass]="{ 'content-height-dev': !isProduction || isTestServer, 'is-answer-thread-open': Boolean(postInThread) }"This change makes the boolean conversion more explicit and potentially more readable for other developers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div class="communication-message-wrap col flex-grow-1 module-bg rounded-3 scrollable-content" [ngClass]="{ 'content-height-dev': !isProduction || isTestServer, 'is-answer-thread-open': Boolean(postInThread) }" >
60-61: 🧹 Nitpick (assertive)
LGTM: Consistent styling with main content area
The updates to the adjacent content area's class structure are consistent with the changes made to the main content area. This consistency improves the overall structure and maintainability of the template.
As suggested for the main content area, consider simplifying the
ngClassbinding:- [ngClass]="{ 'content-height-dev': !isProduction || isTestServer, 'is-answer-thread-open': !!postInThread }" + [ngClass]="{ 'content-height-dev': !isProduction || isTestServer, 'is-answer-thread-open': Boolean(postInThread) }"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.class="communication-answer-message-wrap col flex-grow-1 justify-end px-0 scrollable-content" [ngClass]="{ 'content-height-dev': !isProduction || isTestServer, 'is-answer-thread-open': Boolean(postInThread) }"
25-26: 🧹 Nitpick (assertive)
LGTM: Enhanced sidebar styling based on conversation state
The addition of the
is-not-in-active-conversationclass when there's no active conversation is a good improvement. It allows for more dynamic styling of the sidebar based on the application state.Consider using kebab-case for CSS class names to align with common CSS naming conventions:
- 'is-not-in-active-conversation': !activeConversation + 'is-not-in-active-conversation': !activeConversationCommittable suggestion was skipped due to low confidence.
src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.html (1)
45-52:
⚠️ Potential issueImproved date display structure and logic.
The new structure for date display enhances semantic meaning and styling capabilities. The conditional rendering for the "today" flag and the adaptive date format improve user experience.
Use new Angular syntax consistently.
The changes use both old (*ngIf) and new (@if) Angular syntax. As per the coding guidelines, @if and @for should always be used over the old style.
Please update the following lines to use the new Angular syntax:
- <span [disableTooltip]="postingIsOfToday" ngbTooltip="{{ posting.creationDate | artemisDate: 'time' }}"> + <span @if="!postingIsOfToday" [attr.ngbTooltip]="posting.creationDate | artemisDate: 'time'">This change ensures consistency with the new Angular syntax throughout the template.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<span class="post-header-date-separator">-</span> <span class="post-header-date"> @if (postingIsOfToday) { <span [jhiTranslate]="todayFlag ?? ''" id="today-flag" class="fs-small"></span>, } <span @if="!postingIsOfToday" [attr.ngbTooltip]="posting.creationDate | artemisDate: 'time'"> {{ postingIsOfToday ? (posting.creationDate | artemisDate: 'time') : (posting.creationDate | artemisDate: 'short') }} </span>src/main/webapp/app/core/core.module.ts (1)
113-115: 🧹 Nitpick (assertive)
LGTM: Mobile detection and tooltip disabling.
The added code successfully detects mobile devices and disables tooltips accordingly, which aligns with the PR objective of improving the mobile user experience. The implementation follows Angular style guidelines and uses good practices like optional chaining.
Consider adding a brief comment explaining why tooltips are disabled on mobile devices for better code clarity:
// Disable tooltips on mobile devices to improve touch interface usability if (isMobile(window.navigator.userAgent).any ?? false) { tooltipConfig.disableTooltip = true; }src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html (1)
45-52: 🧹 Nitpick (assertive)
Approve changes with a suggestion for syntax improvement
The new structure for date display is well-organized and aligns with the PR objectives of improving the UI. The conditional rendering for the "today" flag enhances the user experience.
However, to adhere to the coding guidelines, consider updating the
@ifdirective to use the new Angular syntax:- @if (postingIsOfToday) { - <span [jhiTranslate]="todayFlag ?? ''" id="today-flag" class="fs-small"></span>, - } + @if (postingIsOfToday) { + <span [jhiTranslate]="todayFlag ?? ''" id="today-flag" class="fs-small"></span>, + }This change ensures consistency with the new Angular syntax as specified in the coding guidelines.
Committable suggestion was skipped due to low confidence.
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts (1)
97-100: 🧹 Nitpick (assertive)
Consider adding return type and JSDoc comment.
The
openSidebarmethod is correctly implemented and follows good practices. However, consider the following improvements:
- Add an explicit return type (
:void) for clarity.- Include a JSDoc comment to describe the method's purpose.
Here's an example of how it could look:
/** * Opens the course sidebar. * This method delegates to the CourseSidebarService. */ public openSidebar(): void { this.courseSidebarService.openSidebar(); }These changes would enhance code readability and maintain consistency with TypeScript and Angular best practices.
src/main/webapp/i18n/en/metis.json (2)
97-97: 🧹 Nitpick (assertive)
LGTM: Added mobile-specific call-to-action.
The new "mobileDisclaimerCallToAction" key provides a clear action for mobile users, which complements the mobile disclaimer message and supports the PR's goal of improving mobile usability.
For consistency with other action phrases in the file (e.g., "Show content", "Collapse content"), consider capitalizing both words:
- "mobileDisclaimerCallToAction": "Select conversation" + "mobileDisclaimerCallToAction": "Select Conversation"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."mobileDisclaimerCallToAction": "Select Conversation"
96-96: 🧹 Nitpick (assertive)
LGTM: Added mobile-specific guidance message.
The new "mobileDisclaimer" key provides clear instructions for mobile users, which aligns with the PR's goal of improving mobile usability.
Consider adding a period at the end of the message for consistency with other messages in the file:
- "mobileDisclaimer": "Please select a conversation first.", + "mobileDisclaimer": "Please select a conversation first.",Committable suggestion was skipped due to low confidence.
src/main/webapp/i18n/de/metis.json (1)
97-97: 🧹 Nitpick (assertive)
LGTM with a minor suggestion: Added mobile-specific call-to-action.
The new "mobileDisclaimerCallToAction" string effectively prompts mobile users to choose a conversation, enhancing the mobile user experience. The German translation correctly uses informal language (dutzen) as per our coding guidelines.
For consistency with the "mobileDisclaimer" string, consider using "Konversation auswählen" instead of "Konversation wählen". This would maintain the same verb ("auswählen") across both related strings.
src/main/webapp/app/overview/course-overview.component.ts (1)
209-224: 🧹 Nitpick (assertive)
LGTM! Ensure proper unsubscription in ngOnDestroy().
The implementation of the CourseSidebarService and the setup of subscriptions in ngOnInit() are well done. The logic for managing the sidebar state is correct and takes into account the activatedComponentReference, which is good for maintaining consistency across components.
To prevent potential memory leaks, ensure that these subscriptions are properly unsubscribed in the ngOnDestroy() method. You can do this by adding the following lines to the ngOnDestroy() method:
ngOnDestroy() { // ... existing code ... this.closeSidebarEventSubscription?.unsubscribe(); this.openSidebarEventSubscription?.unsubscribe(); this.toggleSidebarEventSubscription?.unsubscribe(); }This will ensure that the subscriptions are cleaned up when the component is destroyed.
src/test/javascript/spec/component/course/course-overview.component.spec.ts (1)
656-662: 🧹 Nitpick (assertive)
LGTM! Consider enhancing test robustness.
The test case effectively verifies the basic functionality of opening the dropdown. It correctly checks that the isOpen property is set to true when the dropdown is opened.
To improve the test's robustness and clarity, consider adding an initial state check and using a more descriptive test name:
it('should open the dropdown and set isOpen to true', () => { fixture.detectChanges(); if (component.itemsDrop) { // Check initial state expect(component.itemsDrop.isOpen).toBeFalse(); component.itemsDrop.open(); fixture.detectChanges(); expect(component.itemsDrop.isOpen).toBeTrue(); } else { fail('itemsDrop is not defined'); } });This approach ensures that the initial state is verified, provides a more descriptive test name, and handles the case where itemsDrop might be undefined.
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.html (3)
16-70: 🛠️ Refactor suggestion
Refactor repetitive checkbox code into a reusable component.
The checkbox inputs for filtering options are repetitive. Consider creating a reusable component for these checkboxes to improve maintainability and reduce code duplication.
For example, you could create a
FilterCheckboxComponentthat accepts inputs likeformControlName,labelTranslateKey, andid. This would also make it easier to add or modify filters in the future.
58-63: 🛠️ Refactor suggestion
Simplify tooltip logic by moving it to the component class.
The inline conditional logic within the
[ngbTooltip]attribute can make the template harder to read and maintain. Consider moving this logic to the component class.In your component class, add a getter method:
get sortingOrderTooltip(): string { return this.sortingOrder === SortDirection.ASCENDING ? this.translateService.instant('artemisApp.metis.overview.sortAscending') : this.translateService.instant('artemisApp.metis.overview.sortDescending'); }Then, update the template:
<fa-icon [icon]="sortingOrder === SortDirection.ASCENDING ? faLongArrowAltUp : faLongArrowAltDown" [ngbTooltip]="sortingOrderTooltip" />
76-80: 🧹 Nitpick (assertive)
Enhance accessibility for loading indicator.
The loading indicator lacks ARIA attributes which are important for screen reader accessibility. Consider adding
role="status"andaria-live="polite"to inform assistive technologies of the loading state.<div class="envelope" + role="status" + aria-live="polite"> <fa-icon size="3x" [icon]="faCircleNotch" animation="spin" /> </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.@if (isFetchingPosts) { <div class="envelope" role="status" aria-live="polite"> <fa-icon size="3x" [icon]="faCircleNotch" animation="spin" /> </div> }src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (3)
24-25: 🛠️ Refactor suggestion
Consider using Angular's
BreakpointObserverfor mobile detectionInstead of using the third-party
ismobilejs-es5library for mobile device detection, consider using Angular'sBreakpointObserverfrom@angular/cdk/layout. This approach aligns with Angular best practices, reduces external dependencies, and provides a more robust solution for responsive design.Here's how you can implement it:
Import
BreakpointObserverandBreakpointState:import { BreakpointObserver, BreakpointState } from '@angular/cdk/layout';Inject
BreakpointObserverin the constructor:constructor( private breakpointObserver: BreakpointObserver, // ... other dependencies ) {}Set up the breakpoint observer in
ngOnInit:ngOnInit(): void { this.breakpointObserver .observe(['(max-width: 768px)']) .subscribe((state: BreakpointState) => { this.isMobile = state.matches; }); // ... rest of your code }This uses a media query to detect screen sizes typical of mobile devices. Adjust
(max-width: 768px)as needed for your application's breakpoints.
78-80: 🛠️ Refactor suggestion
Simplify subscription management with a
SubscriptioncontainerWhen handling multiple subscriptions, you can streamline your code by using a
Subscriptioncontainer. This approach simplifies the unsubscription process and enhances code readability.
Declare a composite subscription at the class level:
private subscriptions = new Subscription();Add subscriptions to the container in
ngOnInit:ngOnInit(): void { this.subscriptions.add( this.courseSidebarService.openSidebar$.subscribe(() => this.setIsCollapsed(true)), ); this.subscriptions.add( this.courseSidebarService.closeSidebar$.subscribe(() => this.setIsCollapsed(false)), ); this.subscriptions.add( this.courseSidebarService.toggleSidebar$.subscribe(() => this.toggleSidebar()), ); // ... rest of your code }Unsubscribe from all in
ngOnDestroy:ngOnDestroy() { this.subscriptions.unsubscribe(); this.ngUnsubscribe.next(); this.ngUnsubscribe.complete(); this.profileSubscription?.unsubscribe(); }This ensures all subscriptions are properly cleaned up, preventing potential memory leaks.
Also applies to: 153-163, 233-235
289-300: 🧹 Nitpick (assertive)
Add unit tests for new mobile-specific logic
The newly introduced methods and conditional logic for mobile devices, such as
hideSearchTerm,closeSidebarOnMobile, and the updates inonSearchandonConversationSelected, are crucial for mobile responsiveness. Adding unit tests for these will ensure they function correctly across different scenarios.Would you like assistance in creating unit tests for these methods to enhance test coverage and reliability?
Also applies to: 330-333, 338-342
sachmii
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS5 on my smartphone and using the smartphone simulator in the browser.
I really like the transition when clicking on a communication channel and using the button to leave the channel and go back to the overview; it doesn't look abrupt.
However, when I add a channel to the Favorites section, the previously opened channel opens again.
If I haven't opened any channel prior to adding one to my Favorites section, the issue does not occur.
Good catch with the favorites! Should be fixed with this commit. 👍🏼 |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Show resolved
Hide resolved
Feras797
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
I tested on TS1 using the Iphone 14 pro max simulator in Chrome. Love the changes. No issues were found. One thing I did notice was, when I tried to hide a channel which is being marked as favorite, that channel stays in favorites instead of being moved to hidden. Although I think this has been discussed/resolved in another pull request. (PR #9473)
Checklist
General
Client
Motivation and Context
The communication module is currently not optimized for mobile devices, resulting in UI elements being clipped. While native applications are available, some users may still attempt to access the module via smartphones. Addressing the clipping issue will enhance the module’s accessibility and improve the overall user experience.
Description
I resolved the clipping issues by overlaying the sidebar, search, messages, and answer thread. Additionally, I introduced a button in the conversation title to navigate back to the sidebar. When performing a search, the sidebar now automatically hides. This hide-and-show functionality of the sidebar is managed through a newly implemented event service. To avoid confusion, the "All Messages" tab is hidden on smartphones.
Most of the changes were made in CSS. However, for the sidebar functionality, the user agent is checked to determine if the device is a smartphone. These changes are designed to have no impact on the desktop version of the application.
This update serves as a quick fix to ensure basic usability of the module, including channel selection, reading and writing messages, and viewing answer threads.
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
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests