-
Notifications
You must be signed in to change notification settings - Fork 297
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
Learning path
: Introduce loading spinner for navigation between learning objects
#9500
Learning path
: Introduce loading spinner for navigation between learning objects
#9500
Conversation
WalkthroughThe changes in this pull request involve updates to 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
|
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
🧹 Outside diff range and nitpick comments (11)
src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.scss (1)
15-17
: LGTM! Consider using a CSS variable for width.The
.loading-icon-container
class is well-implemented and aligns with the PR objective of adding a loading spinner. The width of 15px is appropriate for a small loading icon.For improved flexibility and maintainability, consider using a CSS variable for the width:
.loading-icon-container { - width: 15px; + width: var(--loading-icon-width, 15px); }This allows for easy adjustments across the application if needed.
src/main/webapp/app/course/learning-paths/services/learning-path-navigation.service.ts (1)
8-9
: LGTM: Improved dependency injection and signal declarationsThe changes to the service properties are well-implemented:
- Using
inject()
for dependency injection improves code readability.- Simplifying signal declarations makes the code more concise while maintaining functionality.
These updates align with modern Angular practices and our coding guidelines.
Consider adding type annotations to the signal declarations for improved code clarity:
readonly isLoading = signal<boolean>(false); readonly learningPathNavigation = signal<LearningPathNavigationDTO | undefined>(undefined); readonly isCurrentLearningObjectCompleted = signal<boolean>(false);Also applies to: 11-11, 13-14, 16-16
src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.html (2)
7-19
: LGTM! Consider adding aria-label for accessibility.The changes look good. The loading indicator improves user feedback, and the new parameter in
selectLearningObject
aligns with the component changes. The use of@if
adheres to the coding guidelines.Consider adding an
aria-label
to the button for better accessibility:<button id="previous-button" (click)="selectLearningObject(predecessorLearningObject, false)" type="button" class="btn btn-secondary d-flex justify-content-center" + aria-label="Navigate to previous learning object" >
49-57
: LGTM! Consider consistent button structure for better maintainability.The changes look good. The loading indicator improves user feedback, and the new parameter in
selectLearningObject
aligns with the component changes. The use of@if
adheres to the coding guidelines.For better maintainability, consider structuring the next button similarly to the previous button:
-<button id="next-button" (click)="selectLearningObject(successorLearningObject, true)" type="button" class="btn btn-primary d-flex justify-content-center"> +<button + id="next-button" + (click)="selectLearningObject(successorLearningObject, true)" + type="button" + class="btn btn-primary d-flex justify-content-center" + aria-label="Navigate to next learning object" +> <span jhiTranslate="artemisApp.learningPath.navigation.nextButton"></span> <div class="ms-1 loading-icon-container"> @if (isLoadingSuccessor()) { <fa-icon [icon]="faSpinner" animation="spin" /> } @else { <fa-icon [icon]="faChevronRight" /> } </div> </button>This change improves consistency with the previous button and adds an aria-label for accessibility.
src/test/javascript/spec/component/learning-paths/components/learning-path-nav.component.spec.ts (6)
48-64
: LGTM: TestBed configuration updated correctlyThe TestBed configuration has been appropriately updated to use
MockComponent
forLearningPathNavOverviewComponent
and to provide a mockLearningPathNavigationService
. This aligns with best practices for component testing in Angular.Consider using
jest.fn().mockReturnValue(of({}))
for methods that are expected to return Observables, such aslearningPathNavigation
andcurrentLearningObject
. This would more accurately represent the asynchronous nature of these methods in the actual service.
82-88
: LGTM: Updated test case for initial navigation loadingThe test case has been correctly updated to check for the loading of initial learning path navigation using the new
loadLearningPathNavigation
method. The use oftoHaveBeenCalledExactlyOnceWith
is a good practice for ensuring precise method calls.Consider adding an expectation to verify that the component's properties are updated correctly after the navigation is loaded. This would ensure that the component state reflects the loaded data.
119-150
: LGTM: Navigation test cases updated correctlyThe test cases for next and previous button navigation have been well-updated to use the new
loadRelativeLearningPathNavigation
method. The checks for loading state changes are a good practice for ensuring proper UI feedback during navigation.Consider adding expectations to verify that the component's learning object properties (predecessor, current, successor) are updated correctly after navigation. This would ensure that the component state reflects the new navigation state.
153-167
: Good addition of complete button test case, but could be improvedThe addition of a test case for the complete button functionality is good. It correctly checks for the call to
completeLearningPath
.Consider enhancing this test case:
- Verify that the component's state is updated correctly after completion (e.g., progress should be 100%).
- Check if any navigation occurs after completion (e.g., moving to the next learning object).
- Ensure that the UI reflects the completed state (e.g., button disabled or hidden).
169-174
: Good addition of navigation overview test, but could be expandedThe addition of a test case for showing the navigation overview is a good start. It correctly verifies that
isDropdownOpen
is set.Consider enhancing this test case:
- Verify that the navigation overview component is actually rendered when
isDropdownOpen
is true.- Add a test to check that the overview is hidden when
isDropdownOpen
is false.- Test the toggle functionality if there's a method to switch the dropdown state.
Line range hint
1-174
: Overall assessment: Well-updated test suite with room for minor enhancementsThe test suite has been successfully updated to accommodate the new
LearningPathNavigationService
and the changes in component functionality. The test cases cover the main aspects of the component's behavior, including navigation, progress tracking, and UI interactions.To further improve the test suite:
- Consider adding more assertions to verify component state changes after service method calls.
- Enhance UI-related tests to check for the actual rendering of elements, not just property changes.
- Add edge case scenarios, such as error handling and boundary conditions for navigation.
These enhancements would make the test suite more robust and comprehensive, ensuring better code quality and easier maintenance in the future.
src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.ts (1)
Line range hint
58-60
: Add error handling to thecompleteLearningPath
methodThe
completeLearningPath
method calls a service without handling potential errors. To prevent unhandled exceptions and improve user feedback, consider adding a try-catch block around the service call.Apply this diff to add error handling:
completeLearningPath(): void { + try { this.learningPathNavigationService.completeLearningPath(); + } catch (error) { + // Handle error appropriately, e.g., show a notification to the user + console.error('Error completing learning path:', error); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.html (3 hunks)
- src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.scss (1 hunks)
- src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.ts (2 hunks)
- src/main/webapp/app/course/learning-paths/services/learning-path-navigation.service.ts (1 hunks)
- src/test/javascript/spec/component/learning-paths/components/learning-path-nav.component.spec.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.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/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.ts (1)
src/main/webapp/app/course/learning-paths/services/learning-path-navigation.service.ts (1)
src/test/javascript/spec/component/learning-paths/components/learning-path-nav.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/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.scss (1)
Line range hint
1-13
: Additional style changes look good.The changes to
.progress
,.dropdown-toggle::after
, and.dropdown-menu
classes enhance the overall styling and user experience of the component:
- Adjusting the margins for
.progress
improves its alignment.- Hiding the default dropdown toggle indicator allows for custom styling.
- Setting a fixed width and box shadow for
.dropdown-menu
improves its appearance and consistency.These changes align well with the PR objectives of enhancing user experience and maintaining design consistency.
src/main/webapp/app/course/learning-paths/services/learning-path-navigation.service.ts (5)
1-1
: LGTM: Updated imports for Angular signalsThe import statement has been appropriately updated to include
computed
andsignal
from Angular core. This change aligns with modern Angular practices for efficient state management.
Line range hint
18-28
: LGTM: Proper implementation of loading state managementThe
loadLearningPathNavigation
method is well-implemented:
- It correctly manages the loading state using signal setters.
- Error handling is properly implemented with the alert service.
- The method aligns with the PR objectives by providing visual feedback during navigation.
Line range hint
30-42
: LGTM: Correct implementation of relative navigationThe
loadRelativeLearningPathNavigation
method is correctly implemented:
- It handles navigation between learning objects as per the PR objectives.
- The method signature correctly includes all necessary parameters for relative navigation.
- Error handling is properly implemented with the alert service.
- The navigation state is updated using the appropriate signal setter.
Line range hint
44-51
: LGTM: Concise implementation of completion methodsThe
completeLearningPath
andsetCurrentLearningObjectCompletion
methods are well-implemented:
- Both methods use signal setters consistently to update state.
- The implementations are concise and focused, reducing the likelihood of errors.
- They align with the PR objectives by managing the completion state of learning objects.
Line range hint
1-51
: Overall: Excellent implementation of LearningPathNavigationServiceThe changes to this service are well-implemented and align with both the PR objectives and our coding guidelines. Key points:
- Proper use of Angular signals for efficient state management.
- Consistent error handling and state updates throughout the service.
- Clear and concise method implementations that fulfill the required functionality.
The service now provides a robust foundation for managing learning path navigation, including loading states and completion tracking. Great job on improving the code quality and maintainability!
src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.html (2)
Line range hint
1-75
: Overall, great improvements to user feedback and adherence to guidelines.The changes consistently enhance user experience by adding loading indicators and using the new
@if
syntax as per coding guidelines. The modifications toselectLearningObject
method calls align well with the component changes mentioned in the summary.A few minor suggestions have been made for accessibility and consistency, which would further improve the code quality. Great work on this update!
28-37
: LGTM! Consider performance impact of always rendering the button.The changes simplify the template and improve readability. The use of
@if
adheres to the coding guidelines.Consider the performance impact of always rendering the button, especially if there are many items. If performance is a concern, you might want to keep the outer conditional check. To verify the number of items, run the following script:
If the number of items is small, the current implementation is fine.
✅ Verification successful
Verified: Performance Impact Minimal with Few Learning Objects
The number of learning objects is small (3), so the performance impact of always rendering the button is negligible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the number of learning objects in the component # Test: Search for array declarations or assignments related to learning objects rg -A 5 'learningObjects\s*=|learningObjects:\s*\[' src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/Length of output: 133
Script:
#!/bin/bash # Description: Search for declarations of currentLearningObject in the component # Test: Search for function or variable declarations related to currentLearningObject rg 'currentLearningObject\s*\(' src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/Length of output: 666
Script:
#!/bin/bash # Description: Count the number of learning objects used in the component # Test: Extract all instances where learning objects are utilized rg -o 'currentLearningObject\(\)' src/main/webapp/app/course/learning-paths/components/learning-path-student-nav/ | wc -lLength of output: 125
src/test/javascript/spec/component/learning-paths/components/learning-path-nav.component.spec.ts (5)
7-8
: LGTM: Appropriate imports addedThe new imports for
LearningPathNavigationService
andMockComponent
are appropriate for the changes made in this test file. UsingMockComponent
from ng-mocks is a good practice for mocking components in Angular tests.
13-14
: LGTM: Test setup updated correctlyThe test setup has been appropriately updated to use
LearningPathNavigationService
instead of the previous API service. Creating a spy for thelearningPathNavigation
method is a good practice for verifying service interactions in the tests.
70-71
: LGTM: Service injection and spy setupThe injection of
LearningPathNavigationService
and the creation of a spy for thelearningPathNavigation
method are well-implemented. This setup facilitates effective mocking and verification of service method calls in the subsequent tests.
92-98
: LGTM: Progress bar test case updated correctlyThe progress bar test case has been well-updated to use the
learningPathNavigationSpy
for mocking the return value. Checking the width of the progress bar element is an effective way to verify the visual representation of progress.
101-106
: LGTM: learningPathProgress test case updated correctlyThe test case for
learningPathProgress
has been appropriately updated to use thelearningPathNavigationSpy
for mocking the return value. It correctly verifies that the component'slearningPathProgress
property is set to the expected value.
...e/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.ts
Show resolved
Hide resolved
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.
Tested on TS4. Everything works as described & expected. 👍
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.
Code looks good 👍
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.
Code LGTM
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.
Tested on TS5. Works as expected, nice detail
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.
Maintainer approved
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.
Tested on TS2, showed me the small loading spinner when going back and forth. ✅
Checklist
General
Client
Motivation and Context
Although loading times when navigating between adjacent learning objects within the learning path feature are already quite fast, there is still a brief period where the user does not receive any visual feedback indicating that the system is loading or processing.
Description
This PR adds a small loading spinner next to the navigation buttons to inform the user when the system is loading the next or previous learning object. Additionally, the tests for the learning path navigation have been refactored for improved accuracy.
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
Client
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Tests