-
Notifications
You must be signed in to change notification settings - Fork 358
Communication: Simplify mentioning empty lectures
#12030
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
base: develop
Are you sure you want to change the base?
Communication: Simplify mentioning empty lectures
#12030
Conversation
|
@anian03 Test coverage has been automatically updated in the PR description. |
|
@anian03 Test coverage has been automatically updated in the PR description. |
|
@anian03 Test coverage has been automatically updated in the PR description. |
|
@anian03 Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
|
@anian03 Test coverage has been automatically updated in the PR description. |
WalkthroughConditional menu triggering was added to the markdown editor: lecture and unit menu triggers now open submenus only when referencable attachments or slides exist, otherwise they execute the current editor action. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: 1
🤖 Fix all issues with AI agents
In
`@src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html`:
- Around line 187-202: The template currently checks unit.slides truthiness
which treats empty arrays as true; change the conditional on the
matMenuTriggerFor and the click guard to use unit.slides?.length (e.g.,
[matMenuTriggerFor]="unit.slides?.length ? lectureMenuUnitsSlide : null" and the
click short-circuit to !unit.slides?.length) so the submenu
(lectureMenuUnitsSlide) and the click handler using
displayedActions.lecture.executeInCurrentEditor only run when slides exist;
update both occurrences that reference unit.slides in the <button> to use
unit.slides?.length instead.
🧹 Nitpick comments (1)
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
722-734: Check for non-emptyattachmentsarray rather than just truthiness.The condition
!!lecture.attachmentson line 728 will returntruefor an empty array[], potentially causing the submenu to open when there are no attachments to display. Consider checking the array length:♻️ Suggested improvement
hasReferencableAttachments(lecture: LectureWithDetails): boolean { - const hasAttachments = !!lecture.attachments; + const hasAttachments = !!lecture.attachments?.length; const hasReferencableAttachmentVideoUnits = lecture.attachmentVideoUnits?.some((unit) => { return unit.attachment && unit.attachment.link; - }) === true; + }) ?? false; return hasAttachments || hasReferencableAttachmentVideoUnits; }Also, the
=== truecomparison is redundant since?.some()already returnsboolean | undefined, and the nullish coalescing (?? false) is cleaner than strict equality.
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html
Show resolved
Hide resolved
|
@anian03 Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
|
@anian03 Test coverage has been automatically updated in the PR description. |
|
@anian03 Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
matyasht
left a comment
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.
looks good and works as described
Checklist
General
Client
Motivation and Context
When mentioning lectures in communication, there can be multiple nested menus. For empty lectures, this often makes no sense as you need to select the lecture twice in order to create a content mention.
Description
For lectures that don't have attachments/units we can mention, we remove the sub-menu for selecting units/attachments as it does not provide any functionality and only adds an additional step for users.
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Client
Last updated: 2026-01-25 18:27:16 UTC
Screenshots
Before
After
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.