Communication: Fix element height in announcement channel
#9664
Conversation
PaRangger
added
client
github-actions
bot
removed
the
communication
PaRangger
changed the title
Communication: Fix messages element height in announcement channel
PaRangger
changed the title
Communication: Fix messages element height in announcement channelCommunication: Fix element height in announcement channel
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
PaRangger
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe changes in this pull request involve modifications to the HTML, SCSS, and TypeScript files of the 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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.scss (2)
50-52: Consider adding responsive design for hide-input-full modifier.The significant height increase to 94vh might cause issues on smaller screens or mobile devices. Consider adding media queries similar to the existing ones to ensure proper display across all viewport sizes.
&.hide-input-full { max-height: calc(94vh - var(--message-input-height-prod) - var(--search-height) - var(--channel-header-height)); + @include media-breakpoint-down(sm) { + max-height: calc(90vh - var(--message-input-height-prod) - var(--search-height) - var(--channel-header-height)); + max-height: calc(90dvh - var(--message-input-height-prod) - var(--search-height) - var(--channel-header-height)); + } }
54-56: Maintain consistent responsive behavior across modifiers.Similar to the hide-input-full modifier, consider adding media queries for consistent responsive behavior.
&.hide-input { max-height: calc(87vh - var(--message-input-height-prod) - var(--search-height) - var(--channel-header-height)); + @include media-breakpoint-down(sm) { + max-height: calc(90vh - var(--message-input-height-prod) - var(--search-height) - var(--channel-header-height)); + max-height: calc(90dvh - var(--message-input-height-prod) - var(--search-height) - var(--channel-header-height)); + } }src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html (1)
70-80: Consider extracting complex conditions into component methods.While the implementation correctly addresses the height issue in announcement channels, the ngClass conditions are quite complex. Consider extracting these checks into readable component methods:
- [ngClass]="{ - 'posting-infinite-scroll-container': posts.length !== 0, - 'content-height-dev': contentHeightDev, - 'is-fetching-posts': isFetchingPosts, - 'hide-input-full': - getAsChannel(_activeConversation)?.isAnnouncementChannel && - _activeConversation !== undefined && - !canCreateNewMessageInConversation(_activeConversation), - 'hide-input': - getAsChannel(_activeConversation)?.isAnnouncementChannel && _activeConversation !== undefined && canCreateNewMessageInConversation(_activeConversation), - }" + [ngClass]="getMessageContainerClasses()"Add to component:
getMessageContainerClasses() { return { 'posting-infinite-scroll-container': this.posts.length !== 0, 'content-height-dev': this.contentHeightDev, 'is-fetching-posts': this.isFetchingPosts, 'hide-input-full': this.shouldHideInputFull(), 'hide-input': this.shouldHideInput() }; } private shouldHideInputFull(): boolean { return this.isAnnouncementChannel() && !this.canCreateNewMessageInConversation(this._activeConversation); } private shouldHideInput(): boolean { return this.isAnnouncementChannel() && this.canCreateNewMessageInConversation(this._activeConversation); } private isAnnouncementChannel(): boolean { return this._activeConversation !== undefined && this.getAsChannel(this._activeConversation)?.isAnnouncementChannel; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.scss(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
🔇 Additional comments (3)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.scss (1)
Line range hint 50-74: Implementation successfully addresses the PR objectives.
The height adjustments effectively solve the empty space issue in the announcement channel while maintaining the existing responsive design patterns. The implementation is clean and follows the component's established patterns.
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html (2)
Line range hint 1-150: Template syntax follows latest Angular guidelines.
The template correctly uses the new @if and @for syntax throughout, adhering to the coding guidelines.
70-80: Verify the height adjustment in different scenarios.
The implementation looks correct, but let's verify it works as expected in all scenarios:
#!/bin/bash
# Description: Verify the CSS classes are properly defined and used consistently
# Test 1: Check if the new CSS classes are defined in the SCSS file
echo "Checking for CSS class definitions..."
rg -A 2 'hide-input-full|hide-input' 'src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.scss'
# Test 2: Check for any other components that might be affected by these height changes
echo "Checking for related height adjustments in other components..."
rg -A 2 'posting-infinite-scroll-container' 'src/main/webapp/**/*.scss'
...rview/course-conversations/layout/conversation-messages/conversation-messages.component.scss
Show resolved
Hide resolved
...rview/course-conversations/layout/conversation-messages/conversation-messages.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (2)
147-148: LGTM! Consider enhancing test coverage.The new assertions correctly validate the UI state for announcement channels. The test verifies that:
isHiddenInputFullis false, allowing full height utilizationisHiddenInputWithCallToActionis true, showing the announcement buttonConsider adding these additional test cases to improve coverage:
- Verify the CSS classes are correctly applied to the DOM
- Test the transition between announcement and non-announcement channels
- Add edge cases for different user permission scenarios
Example:
it('should apply correct CSS classes for announcement channels', fakeAsync(() => { const scrollableDiv = fixture.debugElement.query(By.css('.scrollableDiv')); expect(scrollableDiv.classes['hide-input-full']).toBeFalsy(); expect(scrollableDiv.classes['hide-input']).toBeTruthy(); }));
Line range hint
41-161: Enhance test specificity and patterns.While the test structure is solid, consider these improvements to align with the coding guidelines:
- Replace generic
toBeTruthy()assertions with more specific matchers:-expect(announcementButton).toBeTruthy(); +expect(announcementButton).not.toBeNull(); -expect(modal).toBeTruthy(); +expect(modal).not.toBeNull(); -expect(inlineInput).toBeTruthy(); +expect(inlineInput).not.toBeNull();
- Make test descriptions more specific:
-it('should create', fakeAsync(() => { +it('should create component and handle scroll on new message', fakeAsync(() => { -it('should set initial values correctly', fakeAsync(() => { +it('should initialize course, active conversation, and posts', fakeAsync(() => {src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
84-85: LGTM! Consider adding property documentation.The property names are descriptive and follow the Angular style guide. Consider adding JSDoc comments to document their purpose and relationship to the announcement channel functionality.
/** Controls visibility of input with call-to-action for announcement channels */ isHiddenInputWithCallToAction = false; /** Controls full input visibility for announcement channels */ isHiddenInputFull = false;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.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}}
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Show resolved
Hide resolved
|
github-actions
bot
added
the
deployment-error
github-actions
bot
removed
the
deployment-error
vinceclifford
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
deleted the
bugfix/communication/hide-black-bar-in-announcement-channel
branch
Checklist
General
Client
Motivation and Context
Currently, when accessing the announcement channel as a student, a big empty bar is at the bottom where the message input is supposed to be. This should not be there and instead the announcements should take over the entire height of the element.
Addresses #9597
Description
I added a check to see if the user is in the announcement channel and if so added a class that extends the height of the message area.
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
Instructor:
Student:
Summary by CodeRabbit
New Features
Style
Tests