Communication: Improve design of hover area on messages
#9963
Conversation
asliayk
added
tests
client
WalkthroughThe changes in this pull request primarily focus on enhancing the visual presentation and interaction of answer posts and their headers within the application. Modifications include the introduction of new CSS classes for hover effects, adjustments to conditional rendering based on post timing, and updates to the grouping logic for consecutive posts. Additionally, the test suites for the affected components have been updated to include new test cases that validate these changes, ensuring that the display logic and grouping functionality work as intended. Changes
Assessment against linked issues
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html (1)
34-38: Consider moving date format logic to the component.The template contains complex date formatting logic that could be simplified by moving it to a component method. This would improve maintainability and make the template cleaner.
Consider refactoring like this:
- {{ - postingIsOfToday - ? (posting.creationDate | artemisDate: 'time') - : (posting.creationDate | artemisDate: 'short-date') + ' - ' + (posting.creationDate | artemisDate: 'time') - }} + {{ getFormattedDate(posting.creationDate) }}Add to the component:
getFormattedDate(date: moment.Moment): string { const time = this.artemisDate.transform(date, 'time'); if (this.postingIsOfToday) { return time; } const shortDate = this.artemisDate.transform(date, 'short-date'); return `${shortDate} - ${time}`; }src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.html (1)
34-38: LGTM! Consider using i18n for the date-time separator.The implementation correctly handles the different date display formats based on whether the post is from today, which aligns with the PR objectives.
Consider moving the " - " separator to a translation key for better i18n support:
- : (posting.creationDate | artemisDate: 'short-date') + ' - ' + (posting.creationDate | artemisDate: 'time') + : (posting.creationDate | artemisDate: 'short-date') + ('artemisApp.metis.post.dateTimeSeparator' | artemisTranslate) + (posting.creationDate | artemisDate: 'time')src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (2)
Line range hint
140-149: Consider optimizing change detectionWhile the manual change detection is necessary here, consider implementing OnPush change detection strategy to optimize performance, especially since the grouping operation could be expensive with large datasets.
Add the following to the @component decorator:
@Component({ selector: 'jhi-post-footer', templateUrl: './post-footer.component.html', styleUrls: ['./post-footer.component.scss'], + changeDetection: ChangeDetectionStrategy.OnPush })
Line range hint
140-149: Optimize memory usage and array operationsThe current implementation could be optimized to prevent potential memory leaks and improve performance:
- Consider caching the grouped posts to avoid unnecessary recalculations
- Avoid creating new arrays with reverse().map() when possible
Here's a suggested optimization:
private cachedPosts: string | null = null; private cachedGroups: PostGroup[] = []; groupAnswerPosts(): void { // Create a cache key based on posts const postsKey = this.sortedAnswerPosts.map(p => `${p.id}-${p.creationDate}`).join('|'); // Return cached result if available if (this.cachedPosts === postsKey) { this.groupedAnswerPosts = this.cachedGroups; return; } // Process posts only if necessary if (!this.sortedAnswerPosts?.length) { this.groupedAnswerPosts = []; this.cachedPosts = null; return; } // Use existing array instead of creating new ones const sortedPosts = this.sortedAnswerPosts; for (let i = 0; i < sortedPosts.length; i++) { const post = sortedPosts[i]; post.creationDateDayjs = post.creationDate ? dayjs(post.creationDate) : undefined; } // ... rest of the grouping logic ... this.cachedPosts = postsKey; this.cachedGroups = groups; this.groupedAnswerPosts = groups; this.changeDetector.detectChanges(); }src/main/webapp/app/shared/metis/answer-post/answer-post.component.scss (2)
60-67: Consider using CSS custom properties for hover backgroundWhile the implementation is correct, consider extracting the background color and transition duration into CSS custom properties for better maintainability.
.hover-container { position: relative; } .hover-container:hover { - background: var(--metis-selection-option-hover-background); - transition: background-color 0.2s ease-in-out; + background: var(--hover-background, var(--metis-selection-option-hover-background)); + transition: background-color var(--hover-transition-duration, 0.2s) ease-in-out; }
93-105: Improve post time positioning and visibilityThe absolute positioning of post-time could be improved for better alignment and accessibility.
.post-time { position: absolute; - top: 5%; + top: 50%; left: 0.5rem; margin-left: 0.7rem; + transform: translateY(-50%); visibility: hidden; opacity: 0; transition: visibility 0.2s ease-in-out, opacity 0.2s ease-in-out; font-size: 0.75rem; color: var(--metis-gray-dark); + pointer-events: none; }src/main/webapp/app/shared/metis/answer-post/answer-post.component.html (1)
14-18: Consider adding aria-label for better accessibilityWhile the post time implementation is correct, consider adding an aria-label for screen readers.
- <span class="post-time fs-small" ngbTooltip="{{ posting.creationDate | artemisDate: 'time' }}"> + <span + class="post-time fs-small" + ngbTooltip="{{ posting.creationDate | artemisDate: 'time' }}" + [attr.aria-label]="'Post time: ' + (posting.creationDate | artemisDate: 'time')" + > {{ posting.creationDate | artemisDate: 'time' }} </span>src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts (1)
76-77: Make assertions more specificThe current assertions could be more specific about the expected group sizes.
- expect(component.groupedAnswerPosts.length).toBeGreaterThan(0); - expect(component.groupedAnswerPosts[0].posts.length).toBeGreaterThan(0); + expect(component.groupedAnswerPosts).toHaveLength(2); + expect(component.groupedAnswerPosts[0].posts).toHaveLength(2);src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
394-408: Consider extracting common test logic to a shared helper.While the test implementation is correct, there's significant code duplication with answer-post.component.spec.ts. Consider extracting the common time display testing logic to a shared test helper.
Example shared helper:
export function testPostTimeDisplay(fixture: ComponentFixture<any>, debugElement: DebugElement): void { const fixedDate = dayjs('2024-12-06T23:39:27.080Z'); const component = fixture.componentInstance; component.posting = { ...metisPostExerciseUser1, creationDate: fixedDate }; // Test consecutive case jest.spyOn(component, 'isConsecutive').mockReturnValue(true); fixture.detectChanges(); const postTimeElement = debugElement.query(By.css('span.post-time')); expect(postTimeElement).toBeTruthy(); expect(postTimeElement.nativeElement.textContent?.trim()) .toBe(dayjs(fixedDate).format('HH:mm')); // Test non-consecutive case jest.spyOn(component, 'isConsecutive').mockReturnValue(false); fixture.detectChanges(); expect(debugElement.query(By.css('span.post-time'))).toBeFalsy(); }src/main/webapp/app/shared/metis/post/post.component.scss (3)
35-37: Consider adding position transition for smoother UXThe positioning adjustment for non-consecutive messages is good, but the abrupt position change might be jarring.
Add transition for the top position:
.non-consecutive .hover-actions { top: -2rem; + transition: top 0.2s ease-in-out; }
44-47: Consider adding z-index for proper layeringWhile the visibility toggle works correctly, adding a z-index would ensure the hover actions always appear above other content.
.hover-container:hover .hover-actions { visibility: visible; opacity: 1; + z-index: 1; }
86-88: Consider specifying explicit dimensionsWhile relative positioning is correct, adding explicit width would ensure consistent behavior across different message lengths.
.hover-container { position: relative; + width: 100%; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(4 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.scss(3 hunks)src/main/webapp/app/shared/metis/post/post.component.html(2 hunks)src/main/webapp/app/shared/metis/post/post.component.scss(2 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.html(1 hunks)src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts(4 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(5 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/main/webapp/app/shared/metis/posting-header/post-header/post-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/shared/metis/posting-header/answer-post-header/answer-post-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/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.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/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (1)
src/main/webapp/app/shared/metis/post/post.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/metis/post/post.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/main/webapp/app/shared/metis/answer-post/answer-post.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/metis/answer-post/answer-post.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 (13)
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html (1)
34-38: LGTM! The date/time display logic aligns with requirements.
The implementation correctly shows only time for today's messages and adds the date for older messages, which matches the PR objectives for improving message time visibility.
src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.html (1)
34-38: Verify consistency of date format parameters.
Let's ensure the 'time' and 'short-date' format parameters are used consistently across the application.
✅ Verification successful
Date format parameters are used consistently across the application
The verification shows consistent usage patterns of the 'time' and 'short-date' formats across the application:
- 'time' format is consistently used for displaying time portions of dates
- 'short-date' format is consistently used for displaying calendar dates
- The pattern of combining both formats with " - " separator (as in the reviewed code) is also consistently used in similar contexts (e.g., in answer-post-header.component.html)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of artemisDate pipe with format parameters
rg "artemisDate:\s*['\"](?:time|short-date)['\"]" --type html
Length of output: 8921
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (1)
140-140: LGTM: Time threshold change aligns with requirements
The adjustment of the consecutive message grouping threshold from 1 to 5 minutes aligns with the PR objectives and maintains consistency with the Post component. The implementation includes proper null checks and maintains the existing logic structure.
src/main/webapp/app/shared/metis/answer-post/answer-post.component.scss (1)
Line range hint 24-30: LGTM: Well-structured hover actions styling
The hover actions implementation with max-height, transitions, and proper spacing looks good. The transition timing and easing function provide smooth visual feedback.
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html (1)
1-4: LGTM: Well-structured container with proper class bindings
The main container implementation with hover-container class and conditional classes looks good. The ngClass directive is used appropriately for dynamic class binding.
src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts (1)
165-198: LGTM: Comprehensive test coverage for post grouping
The test implementation thoroughly validates the post grouping logic with:
- Clear test data setup using dayjs
- Proper validation of group sizes and content
- Coverage of different time intervals and authors
src/main/webapp/app/shared/metis/post/post.component.html (2)
2-3: LGTM: Hover container implementation aligns with requirements.
The addition of the hover-container class and the conditional classes in ngClass successfully implements the expanded hover area requirement from issue #9734.
13-17: LGTM: Time display implementation matches requirements.
The conditional time display for consecutive messages aligns with the PR objectives to show message time on hover, similar to Slack's behavior.
src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts (2)
251-266: LGTM: Well-structured test for time display.
The test properly verifies the time display functionality:
- Uses fixed date for consistent testing
- Mocks isConsecutive behavior
- Verifies both element presence and correct time format
268-277: LGTM: Comprehensive negative test case.
The test properly verifies that time is not displayed when messages are not consecutive, completing the test coverage for this feature.
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
410-419: LGTM: Proper negative test case implementation.
The test correctly verifies that time is not displayed for non-consecutive messages, maintaining consistency with the answer-post component tests.
src/main/webapp/app/shared/metis/post/post.component.scss (2)
39-42: LGTM! Clean implementation of hover effect
The background transition is well implemented with proper use of CSS variables and smooth transitions.
49-50: LGTM! Correct positioning context
The relative positioning is correctly set up to support absolutely positioned child elements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/shared/metis/post/post.component.scss (1)
43-51: Add accessibility improvements for hover effectsConsider adding support for users who prefer reduced motion:
.hover-container:hover { background: var(--metis-selection-option-hover-background); transition: background-color 0.2s ease-in-out; + @media (prefers-reduced-motion: reduce) { + transition: none; + } } .hover-container:hover .hover-actions { visibility: visible; opacity: 1; + @media (prefers-reduced-motion: reduce) { + transition: none; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/shared/metis/post/post.component.scss(2 hunks)
🔇 Additional comments (3)
src/main/webapp/app/shared/metis/post/post.component.scss (3)
35-37: LGTM: Relative positioning enables expanded hover area
The .hover-container with relative positioning provides the foundation for expanding the clickable space, which directly addresses the objectives from issue #9734.
90-107: Enhance accessibility for time display
The time display implementation needs the accessibility improvements suggested in the previous review.
39-41: Verify the hover actions positioning
The -2rem top offset for non-consecutive messages might need adjustment based on the actual spacing between messages. Please verify this value works correctly across different message lengths and screen sizes.
✅ Verification successful
The -2rem offset is consistent with the codebase design
The -2rem top offset for non-consecutive messages is intentionally different from the default -1.8rem offset used for regular hover actions. This pattern is consistently implemented across both post and answer-post components, suggesting it's a deliberate design choice to handle different message spacing scenarios. The value appears to be coordinated with other related measurements in the components (like max-height: 2.2rem).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related spacing variables or calculations
rg -t scss "(-1\.8rem|-2rem|2\.2rem)" --glob "!post.component.scss"
Length of output: 98
Script:
#!/bin/bash
# Search for related spacing variables or calculations with correct file extension
rg "(-1\.8rem|-2rem|2\.2rem)" --type css
# Search for hover-actions class usage
rg "hover-actions" --type css -A 3 -B 3
# Search for non-consecutive class usage
rg "non-consecutive" --type css -A 3 -B 3
Length of output: 5816
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
Longppham
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
|
Tested on TS1. When i hover over the messages in consecutive messages, It shows the time like shown in the screenshot. Same applies to answers on a post. Format is also uniform |
ItsaaaMeMario
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
ahbitaqu
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
[Tested on TS3]
Thanks for working on this, small change but a significant quality of life improvement!
Works amazingly :)
Minor suggestion though:
Is there any way to add a slight bit of padding at the top of the first message
Currently, how the box ligns up perfectly with the top of the profile picture feels a little off to me, especially since there is padding in every other direction
Adding this padding could be better, I'll include it in a follow-up PR |
krusche
changed the title
Communication: Improve hovered message designCommunication: Improve design of hover area on messages
krusche
deleted the
feature/communication/improve-hovered-message-design
branch
Checklist
General
Client
Motivation and Context
Communication:Expand clickable space for replying #9734)AnswerPostcomponent, there was an additional spacing issue between consecutive messages compared to thePostcomponent.AnswerPostcomponent was 1 minute, whereas in thePostcomponent, it was 5 minutes.Description
AnswerPostcomponent has been removed.AnswerPostcomponent has been updated from 1 minute to 5 minutes, making it consistent with thePostcomponent.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
hovered message with expanded hover area and sent time on header
hovered consecutive message with sent time
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Tests