General: Animate content sidebar on toggle
#9412
Conversation
github-actions
bot
added
the
client
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces modifications to the HTML and SCSS files for the course overview component and global styles. Key changes include updating a CSS class in the HTML template based on 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: 11
🧹 Outside diff range comments (4)
src/main/webapp/app/overview/course-lectures/course-lectures.component.html (1)
Line range hint
2-17: Correct usage of new Angular syntaxThe template is correctly using the new Angular syntax
@ifinstead of*ngIfas per the coding guidelines. This is applied consistently throughout the template.Animation effect for sidebar toggle not implemented
The PR objectives mention adding an animation effect to the content sidebar when it is toggled. However, I don't see any changes in this template that implement this animation.
To implement the animation effect, you might need to:
- Add CSS classes for the animation.
- Use Angular's animation module to define the transition.
- Bind the animation trigger to the
isCollapsedstate.Would you like assistance in implementing these changes or creating a GitHub issue to track this task?
src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1)
Line range hint
1-67: *Consider updating remaining ngIf directives to @ifWhile reviewing the entire file, I noticed that some *ngIf directives are still being used (e.g., on line 2). For consistency and to align with the coding guidelines, consider updating these to the new @if syntax.
Here's an example of how you could update the *ngIf on line 2:
- <div class="input-group mb-2 rounded-3 p-2 me-2 module-bg" [hidden]="!isCodeOfConductAccepted"> + @if (isCodeOfConductAccepted) { + <div class="input-group mb-2 rounded-3 p-2 me-2 module-bg">Make sure to add a closing brace
}at the appropriate place after the div's closing tag.src/main/webapp/app/overview/course-overview.component.html (1)
Line range hint
1-300: Consider consistent usage of Angular syntax and potential refactoring for improved readability.
Angular Syntax Consistency:
The template uses both old-style (*ngIf/*ngFor) and new-style (@if/@for) Angular syntax. For consistency and to align with the latest Angular best practices, consider updating all instances to use the new @if/@for syntax.Template Complexity:
The template contains multiple nested ng-template elements and a complex structure. While this is not inherently wrong, it might impact readability and maintainability. Consider breaking down complex parts into smaller, reusable components to improve the overall structure and maintainability of the template.These suggestions aim to enhance code consistency and maintainability without changing the functionality.
src/main/webapp/content/scss/themes/_dark-variables.scss (1)
Line range hint
138-143: LGTM: Module and navigation bar styling variables addedThe new variables for module and navigation bar styling are well-defined and maintain consistency with the overall theme. They provide a standardized way to style these components across the application.
Consider renaming
$course-image-bgto$course-image-backgroundfor consistency with other background variable names in the file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (11)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1 hunks)
- src/main/webapp/app/overview/course-exams/course-exams.component.html (1 hunks)
- src/main/webapp/app/overview/course-exercises/course-exercises.component.html (1 hunks)
- src/main/webapp/app/overview/course-lectures/course-lectures.component.html (1 hunks)
- src/main/webapp/app/overview/course-overview.component.html (1 hunks)
- src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar.component.html (1 hunks)
- src/main/webapp/app/shared/sidebar/sidebar.component.scss (0 hunks)
- src/main/webapp/content/scss/global.scss (1 hunks)
- src/main/webapp/content/scss/themes/_dark-variables.scss (1 hunks)
- src/main/webapp/content/scss/themes/_default-variables.scss (1 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/shared/sidebar/sidebar.component.scss
🧰 Additional context used
📓 Path-based instructions (7)
src/main/webapp/app/overview/course-conversations/course-conversations.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/overview/course-exams/course-exams.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/overview/course-exercises/course-exercises.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/overview/course-lectures/course-lectures.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/overview/course-overview.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.
🔇 Additional comments (19)
src/main/webapp/app/overview/course-lectures/course-lectures.component.html (1)
1-1:⚠️ Potential issueRemoval of
gap-3class may affect layoutThe
gap-3class has been removed from the outer<div>element. This change aligns with the PR objectives, which mention replacing the gap property with a margin applied directly to the sidebar. However, I don't see the corresponding margin being added to the sidebar element in this file.To ensure the layout is correct, please verify:
- The margin is applied correctly to the sidebar component.
- The removal of
gap-3doesn't negatively impact the layout of other elements.You can use the following script to check for the margin application in the sidebar component:
✅ Verification successful
Margin styles are correctly applied to sidebar components
The shell script confirmed that margin styles are present in the sidebar component SCSS files, aligning with the PR objectives of replacing
gap-3with margins. The removal of thegap-3class does not negatively impact the layout.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for margin application in the sidebar component # Search for the sidebar component file sidebar_file=$(fd --type f --glob '*sidebar.component.*' src/main/webapp) # Check for margin-related styles in the sidebar component if [ -n "$sidebar_file" ]; then echo "Checking $sidebar_file for margin styles:" rg 'margin|mr-' "$sidebar_file" else echo "Sidebar component file not found." fiLength of output: 8287
Script:
#!/bin/bash # Description: Check for margin application in the sidebar component SCSS files # Search for the sidebar component SCSS files sidebar_scss_files=$(fd --type f --glob '*sidebar.component.scss' src/main/webapp) if [ -n "$sidebar_scss_files" ]; then for file in $sidebar_scss_files; do echo "Checking $file for margin styles:" rg 'margin|mr-' "$file" done else echo "No sidebar component SCSS files found." fiLength of output: 2680
src/main/webapp/app/overview/course-tutorial-groups/course-tutorial-groups.component.html (4)
2-2: Correct usage of new Angular syntaxThe template correctly uses the new
@ifand@elsesyntax instead of*ngIf, which aligns with the provided coding guidelines. This change improves code readability and follows Angular's latest best practices.Also applies to: 8-13
Line range hint
1-20: Overall structure and conditional rendering logic are correctThe template maintains the correct structure and conditional rendering logic after the changes:
- The outer condition checks for the presence of
course.- The sidebar is always rendered when
courseis present.- The main content area is conditionally rendered based on
tutorialGroupSelected.This structure aligns with the PR objectives and maintains the intended functionality.
Line range hint
1-20: Summary of changes and potential impactThe changes in this file are minimal but significant:
- Removal of the
gap-3class from the outer<div>, which may affect the layout spacing.- Correct usage of the new Angular
@ifand@elsesyntax.These changes align with the PR objectives and coding guidelines. The main point of attention is ensuring that the layout remains correct after removing the
gap-3class. Please thoroughly test the component's appearance across different screen sizes and content scenarios to confirm that the spacing between the sidebar and main content area remains appropriate.To verify the overall functionality and appearance:
- Test the sidebar toggle animation as mentioned in the PR objectives.
- Confirm that the margin on the right side of the sidebar appears correctly.
- Check the component's appearance in both light and dark modes.
#!/bin/bash # Description: Check for any animation-related styles or classes # Test: Search for animation-related CSS classes or inline styles rg --type html -e 'class="[^"]*\b(animate|transition)\b' src/main/webapp/app/overview/course-tutorial-groups/ rg --type html -e 'style="[^"]*animation|transition' src/main/webapp/app/overview/course-tutorial-groups/ # Test: Search for any CSS files that might contain the animation styles fd -e css | xargs rg -e 'animation|transition'
1-1: Verify layout after removinggap-3classThe removal of the
gap-3class from the outer<div>aligns with the PR objectives. However, please ensure that this change doesn't negatively impact the layout, especially the spacing between the sidebar and the main content area.To verify the layout:
- Check that the sidebar and main content are properly aligned.
- Ensure there's adequate spacing between these elements.
- Test with different screen sizes to confirm responsiveness.
src/main/webapp/app/overview/course-exercises/course-exercises.component.html (5)
1-1: LGTM on class removal and Angular syntax.The removal of the
gap-3class aligns with the PR objectives. Also, the use of@ifinstead of*ngIfcomplies with the coding guidelines.
1-1: Verify margin implementation on sidebar.The PR objectives mention replacing the gap with a margin applied directly to the sidebar. However, this change is not visible in this file. Please confirm if this has been implemented elsewhere or if it needs to be added here.
Line range hint
1-19: Request information on animation implementation.The PR objectives mention adding an animation effect to the content sidebar when toggled. While the
isCollapsedbinding suggests this functionality, the animation itself is not visible in this HTML. Could you please provide information on where and how the animation is implemented (e.g., in CSS or TypeScript)?
Line range hint
3-5: Request information onDEFAULT_COLLAPSE_STATE.The sidebar component uses
DEFAULT_COLLAPSE_STATEfor itscollapseStateinput. Could you please provide information on what this constant represents and how it relates to the new animation effect mentioned in the PR objectives?
Line range hint
7-18: LGTM on content area structure and rendering logic.The conditional rendering of the content area based on
exerciseSelectedis implemented correctly. The use of@ifand@elsecomplies with the coding guidelines, and the structure for both cases (selected and not selected) appears appropriate.src/main/webapp/app/overview/course-exams/course-exams.component.html (3)
Line range hint
1-19: LGTM: Structure and syntax align with objectives and guidelinesThe overall structure of the template aligns well with the PR objectives. The conditional rendering logic is preserved, and the new Angular syntax (@if and @else) is used consistently, which adheres to the coding guidelines. The layout for different scenarios (exam selected/not selected) is maintained, ensuring the functionality remains intact.
Line range hint
3-5: Verify the implementation of the sidebar animationThe sidebar implementation looks correct, with the
sidebar-collapsedclass toggled based on theisCollapsedstate. However, the animation effect mentioned in the PR objectives is not explicitly visible in this template.Please confirm that the animation is implemented correctly, possibly in a separate CSS file. Run the following script to check for animation-related styles:
#!/bin/bash # Description: Search for animation-related styles for the sidebar # Look for animation, transition, or transform properties in CSS files rg --type css -e '(animation|transition|transform).*sidebar' src/main/webapp/
Line range hint
1-19: Summary: Changes align with objectives, but require verificationThe changes in this file align with the PR objectives, particularly in maintaining the structure for conditional rendering of the sidebar and exam content. The use of new Angular syntax (@if and @else) is correct and follows the coding guidelines.
However, there are two points that require verification:
The removal of the
gap-3class from the outer<div>might affect the layout. Please confirm if this change is intentional and related to the "minor layout adjustments" mentioned in the PR objectives.The animation effect for the sidebar toggle is not explicitly visible in this template. Please provide information on where and how this animation is implemented.
To assist with the verification, please run the following script to check for related changes in other files:
#!/bin/bash # Description: Search for related changes in CSS and TypeScript files # Look for changes in CSS files that might be related to the animation or layout adjustments rg --type css -e '(animation|transition|transform|margin|padding).*sidebar' src/main/webapp/ # Check for changes in the corresponding TypeScript component file cat src/main/webapp/app/overview/course-exams/course-exams.component.tsThis will help ensure that all aspects of the PR objectives are properly implemented and documented.
src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1)
25-25: LGTM! Verify visual alignment.The removal of the 'me-3' class aligns with the PR objective of adjusting the layout to accommodate the new animation effect for the sidebar. This change removes the right margin that previously separated the sidebar from the content.
Please ensure that:
- The sidebar visually aligns correctly with the content area after this change.
- The animation effect for toggling the sidebar works as expected with this layout adjustment.
You can verify this by:
- Logging into Artemis
- Navigating to a course
- Toggling the sidebar to observe the smooth transition effect
- Checking that the margin on the right side of the sidebar appears correctly
src/main/webapp/app/shared/sidebar/sidebar.component.html (2)
1-1: Approved: New wrapper div added. Please clarify its purpose.The addition of the outer
<div class="sidebar-wrap">is likely related to the animation effect mentioned in the PR objectives. While it doesn't introduce any functional changes, it may be used for styling or layout purposes.Could you please provide more details on how this wrapper div contributes to the sidebar animation effect?
5-5: Excellent: Updated to new Angular syntax as per guidelines.Great job on updating the template to use the new
@ifand@fordirectives instead of*ngIfand*ngFor. This change aligns perfectly with the provided coding guidelines, which state that these new directives should always be used over the old style. Your consistent application of this change throughout the template is commendable.Also applies to: 8-8, 11-11, 13-13, 22-22, 32-32, 41-41, 59-59
src/main/webapp/content/scss/themes/_dark-variables.scss (3)
132-134: LGTM: Footer styling variables addedThe new footer-related variables are well-defined and consistent with the existing naming conventions. They provide flexibility for styling the footer in different environments (dev and prod) while maintaining consistency with the dark theme.
133-136: LGTM: Sidebar styling variables addedThe new sidebar-related variables, including
$sidebar-width: 275px, are well-defined and consistent with the PR objectives. They provide a standardized way to style the sidebar and its footer in different environments (dev and prod).
Line range hint
145-162: LGTM: Card and navbar styling variables addedThe new variables for card and navbar styling are well-defined and comprehensive. They offer the following benefits:
- Consistent styling for card components
- Detailed color definitions for various navbar states and elements
- Alignment with Bootstrap's naming conventions, improving maintainability
- Use of existing color variables and SCSS functions, allowing for easy theme adjustments
These additions will greatly enhance the consistency and flexibility of the dark theme across the application.
janthoXO
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
This is a lot of subjective preferences, but for me the sidebar takes too long to collapse. Also I would orientate the animation more on commonly animations like this react animation
There was a problem hiding this comment.
The animation of the general sidebar and the exercise sidebar are not consistent.
The general sidebar has just an extending and simultaneously fade in of the text. Whereas the exercise sidebar first extends and then lets the text fade in.
Screen.Recording.2024-10-03.at.22.16.39.mov
…imate-sidebar-on-toggle
I changed the animation to have no delay between fade and slide. Additionally I made the animation faster 👍🏼 |
It should be possible with ctrl+shift+b. Not with ctrl+shift+m. For me it does not work with ctrl+shift+m though. Could it be a keyboard layout thing? 🤔 I have a german keyboard layout (WERTZ). |
Added a .gif that shows the animation 👌🏻 |
PaRangger
dismissed stale reviews from krusche, coderabbitai[bot], SimonEntholzer, rabeatwork, and janthoXO
via
a2d9b4e
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
src/main/webapp/app/exam/participate/exam-navigation-sidebar/exam-navigation-sidebar.component.html (1)
Line range hint
1-82: Consider these minor improvements for better code style and documentation:
Standardize quote usage: Consider using either single or double quotes consistently throughout the file for attribute values.
Break long lines: Some lines exceed 120 characters. Consider breaking them for better readability. For example:
<span [ngbTooltip]="!examTimeLineView ? ('artemisApp.examParticipation.' + getExerciseButtonTooltip(exercises[i]) | artemisTranslate) : ''">Could be split into:
<span [ngbTooltip]="!examTimeLineView ? ('artemisApp.examParticipation.' + getExerciseButtonTooltip(exercises[i]) | artemisTranslate) : ''">
- Review comments: Consider moving the larger comment blocks (like the one explaining the exam navigation functionality) to a separate documentation file or component metadata, keeping only essential inline comments in the template.
These suggestions are minor and don't affect functionality, but they could improve code maintainability and readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- src/main/webapp/app/exam/participate/exam-navigation-sidebar/exam-navigation-sidebar.component.html (1 hunks)
- src/main/webapp/app/exam/participate/exam-navigation-sidebar/exam-navigation-sidebar.component.scss (1 hunks)
- src/main/webapp/content/scss/global.scss (1 hunks)
🧰 Additional context used
🔇 Additional comments (11)
src/main/webapp/app/exam/participate/exam-navigation-sidebar/exam-navigation-sidebar.component.html (1)
Line range hint
17-17: Excellent use of new Angular syntax!The file correctly uses the new Angular syntax
@ifand@forinstead of*ngIfand*ngFor, as per the coding guidelines. This adoption of the latest Angular features improves code readability and performance.Also applies to: 24-24
src/main/webapp/app/exam/participate/exam-navigation-sidebar/exam-navigation-sidebar.component.scss (3)
110-110: Consistent transform for collapsed menuThe transform property for
.double-arrow.menu-closedcorrectly adjusts the position of the arrow when the menu is collapsed, ensuring a smooth animation effect.
114-115: Smooth transition for sidebar toggleSetting the transition on the
transformproperty in.double-arrowprovides a smooth animation when the sidebar is toggled open or closed.
123-125: Correct rotation for closed menu iconThe
.menu-closed .double-arrow-iconrotation set to0degcorrectly represents the closed state of the sidebar, enhancing user experience.src/main/webapp/content/scss/global.scss (7)
1092-1094: Class renaming enhances clarity.Renaming
.sidebar-collapsedto.sidebar-collapsed-course-overviewimproves specificity and prevents potential styling conflicts across different components.
1096-1098: Transition variables increase maintainability.Defining
$transition-sidebar-content-opacity-length,$transition-sidebar-width-length, and$transition-in-between-delayas variables promotes consistency and simplifies future adjustments to transition timings.
1100-1105: Opacity transition added to.sidebar-content.Implementing an opacity transition for
.sidebar-contentenhances the user experience by providing a smooth fade effect when the sidebar is toggled.
1106-1111: Collapsed sidebar content transitions correctly.Setting the opacity to
0in.sidebar-collapsed .sidebar-contentwith appropriate transitions ensures a seamless fade-out effect when collapsing the sidebar.
1112-1121:.sidebar-wraptransitions support width adjustment.The addition of transitions on
max-widthandmargin-rightin.sidebar-wrapfacilitates a smooth resizing of the sidebar during toggling actions.
1122-1130: Collapsed.sidebar-wraphandles transitions properly.Adjusting
max-widthandmargin-rightto0in.sidebar-collapsed .sidebar-wrapwith corresponding transitions provides a consistent collapsing animation for the sidebar.
1131-1133:.sidebar-widthclass ensures consistent width.Defining the
.sidebar-widthclass with a fixed width promotes consistency across different instances of the sidebar.
...n/webapp/app/exam/participate/exam-navigation-sidebar/exam-navigation-sidebar.component.html
Show resolved
Hide resolved
...n/webapp/app/exam/participate/exam-navigation-sidebar/exam-navigation-sidebar.component.scss
Show resolved
Hide resolved
PaRangger
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
The sidebar to switch between exercises/lectures/communication is animated when toggling it. The sidebar for the content however simply vanishes. For better consistency and a smoother user interface I animated the content sidebar.
Description
I added an animation by gradually lowering the width of the sidebar and lowering the opacity of the items inside the sidebar. To make this smooth some minor layout changes were necessary. I removed the gap property that seperated the sidebar and the content, and instead added a margin directly to the sidebar.
Steps for Testing
Prerequisites:
Screenshots
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
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style