Grading: Scroll down to complaint form after clicking on a complain button #9762
Conversation
github-actions
bot
added
the
client
| * Scrolls to the complaint form | ||
| */ | ||
| private scrollToComplaint(): void { | ||
| this.complaintScrollpoint.nativeElement.scrollIntoView({ behavior: 'smooth' }); |
There was a problem hiding this comment.
Since the element referred here is rendered conditionally, (see resp. html line 20), a suggestion would be to add the "?" construct to check whether nativeElement (or complaintScrollpoint) really exists to prevent runtime errors.
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts
Outdated
Show resolved
Hide resolved
| startComplaint(complainType: ComplaintType): void { | ||
| this.formComplaintType = complainType; | ||
| // Wait for the view to update | ||
| setTimeout(() => this.scrollToComplaint(), 0); |
There was a problem hiding this comment.
Suggestion: for a more "Angular like" approach I would suggest using this.cdr.detectChanges(); (where cdr is ChangeDetectorRef injected through the constructor) instead of setTimeout
badkeyy
force-pushed
the
bugfix/scroll-down-after-clicking-complain
branch
from
c8e3dd1 to
fc3d4c8
Compare
WalkthroughThe changes in this pull request enhance the 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: 2
🧹 Outside diff range and nitpick comments (6)
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.html (1)
62-62: Consider adding ARIA attributes to the scroll anchorWhile the scroll anchor implementation is correct, consider adding ARIA attributes to improve accessibility:
-<div #complaintScrollpoint></div> +<div #complaintScrollpoint role="region" aria-label="Complaint form section"></div>src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts (1)
156-171: Consider adding integration tests for scroll behavior.To ensure the scroll functionality remains reliable:
- Add tests verifying that
openComplaintFormtriggers scrolling- Add tests for error cases when the scroll element isn't available
Example test structure:
describe('ComplaintsStudentViewComponent', () => { it('should scroll to complaint form when openComplaintForm is called', () => { const scrollIntoViewMock = jest.fn(); component.complaintScrollpoint = { nativeElement: { scrollIntoView: scrollIntoViewMock } } as any; component.openComplaintForm(ComplaintType.COMPLAINT); expect(scrollIntoViewMock).toHaveBeenCalledWith({ behavior: 'smooth', block: 'end' }); }); });src/test/javascript/spec/component/complaints/complaint-student-view.component.spec.ts (4)
161-162: Remove unnecessary comment.The comment "Check if button is available" doesn't add value as the expectations clearly show what's being tested.
- //Check if button is available expect(component.complaint).toBeUndefined();
179-180: Improve setTimeout comment.Make the comment more specific about why we're waiting.
- // Wait for setTimeout to execute + // Wait for the scroll animation timeout to complete tick();
261-267: Extract common mock setup.The complaint scrollpoint mock setup is duplicated across tests. Consider extracting it into a helper method.
+ function mockScrollpoint(component: ComplaintsStudentViewComponent): jest.Mock { + const scrollIntoViewMock = jest.fn(); + component.complaintScrollpoint = { + nativeElement: { + scrollIntoView: scrollIntoViewMock, + }, + } as ElementRef; + return scrollIntoViewMock; + } it('should set complaint type MORE_FEEDBACK...', fakeAsync(() => { testInitWithResultStub(of()); component.showSection = true; component.isCorrectUserToFileAction = true; - const scrollIntoViewMock = jest.fn(); - component.complaintScrollpoint = { - nativeElement: { - scrollIntoView: scrollIntoViewMock, - }, - } as ElementRef; + const scrollIntoViewMock = mockScrollpoint(component);Also applies to: 289-295
286-287: Remove unnecessary comment and fix typo.The comment has a typo and doesn't add value as the expectation is self-documenting.
- //Check if button is available expect(component.complaint).toBeUndefined();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.html(3 hunks)src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts(4 hunks)src/test/javascript/spec/component/complaints/complaint-student-view.component.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.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/complaints/complaints-for-students/complaints-student-view.component.ts (1)
src/test/javascript/spec/component/complaints/complaint-student-view.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 (8)
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.html (4)
27-27: LGTM: Improved event handling for complaint button
The change from direct variable assignment to using openComplaintForm method provides a better structure for handling the complaint form interaction, allowing for the implementation of the scrolling behavior.
39-42: LGTM: Consistent implementation for feedback button
Good improvements:
- Added ID for better testability and accessibility
- Consistent event handling pattern with the complaint button
Line range hint 1-71: LGTM: Excellent adherence to Angular syntax guidelines
The template consistently uses the new @if syntax throughout, following the coding guidelines perfectly.
Line range hint 1-71: Verify complete implementation of scrolling behavior
While the template changes provide the necessary structure for the scrolling functionality, we should verify the complete implementation:
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts (3)
56-56: LGTM!
The ChangeDetectorRef injection follows Angular's dependency injection patterns and naming conventions.
156-164: LGTM!
The method implementation follows Angular best practices and achieves the PR objective of improving form visibility.
31-32:
Add optional chaining for safer element access.
Since the element referred by complaintScrollpoint is rendered conditionally, add the optional chaining operator to prevent potential runtime errors.
Apply this diff:
- @ViewChild('complaintScrollpoint') complaintScrollpoint: ElementRef;
+ @ViewChild('complaintScrollpoint') complaintScrollpoint?: ElementRef;src/test/javascript/spec/component/complaints/complaint-student-view.component.spec.ts (1)
151-182: Excellent test coverage for the new scroll functionality!
The tests thoroughly verify both the complaint type setting and scroll behavior in both exam and course modes. Good use of Angular testing patterns with proper async handling and mocking.
Also applies to: 242-305
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/complaints/complaint-student-view.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/complaints/complaint-student-view.component.spec.ts (2)
151-182: Enhance test readability and coverage.Consider the following improvements:
- Make the test title more descriptive, e.g., "should set complaint type and scroll when clicking complaint button in exam mode"
- Extract the button selector as a constant to avoid magic strings
- Add explicit test for button availability instead of just the comment
+const COMPLAINT_BUTTON_SELECTOR = '#complain'; + it('should set complaint type COMPLAINT and scroll to complaint form when pressing complaint', fakeAsync(() => { component.exercise = examExercise; component.result = result; component.exam = defaultExam; component.showSection = true; component.isCorrectUserToFileAction = true; const complaintBySubmissionMock = jest.spyOn(complaintService, 'findBySubmissionId').mockReturnValue(of()); fixture.detectChanges(); - //Check if button is available - expect(component.complaint).toBeUndefined(); - expect(complaintBySubmissionMock).toHaveBeenCalledOnce(); + const button = fixture.debugElement.nativeElement.querySelector(COMPLAINT_BUTTON_SELECTOR); + expect(button).not.toBeNull(); + expect(button.disabled).toBeFalse(); // Mock complaint scrollpoint const scrollIntoViewMock = jest.fn(); component.complaintScrollpoint = { nativeElement: { scrollIntoView: scrollIntoViewMock, }, } as ElementRef; - const button = fixture.debugElement.nativeElement.querySelector('#complain'); button.click(); fixture.detectChanges(); expect(component.formComplaintType).toBe(ComplaintType.COMPLAINT); // Wait for setTimeout to execute tick(); expect(scrollIntoViewMock).toHaveBeenCalledWith({ behavior: 'smooth' }); }));
279-305: Apply consistent improvements across test cases.The same improvements suggested for the previous test cases should be applied here:
- Use the extracted
mockScrollPointhelper- Extract button selector as a constant
- Add explicit button availability test
+const MORE_FEEDBACK_BUTTON_SELECTOR = '#more-feedback'; + it('should set complaint type MORE_FEEDBACK and scroll to complaint form when pressing complaint', fakeAsync(() => { testInitWithResultStub(of()); component.showSection = true; component.isCorrectUserToFileAction = true; fixture.detectChanges(); - //Check if button is available - expect(component.complaint).toBeUndefined(); + const button = fixture.debugElement.nativeElement.querySelector(MORE_FEEDBACK_BUTTON_SELECTOR); + expect(button).not.toBeNull(); + expect(button.disabled).toBeFalse(); - // Mock complaint scrollpoint - const scrollIntoViewMock = jest.fn(); - component.complaintScrollpoint = { - nativeElement: { - scrollIntoView: scrollIntoViewMock, - }, - } as ElementRef; + const scrollIntoViewMock = mockScrollPoint(component); - const button = fixture.debugElement.nativeElement.querySelector('#more-feedback'); button.click(); fixture.detectChanges(); expect(component.formComplaintType).toBe(ComplaintType.MORE_FEEDBACK); tick(); // Wait for setTimeout to execute expect(scrollIntoViewMock).toHaveBeenCalledWith({ behavior: 'smooth' }); }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/complaints/complaint-student-view.component.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/complaints/complaint-student-view.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 (2)
src/test/javascript/spec/component/complaints/complaint-student-view.component.spec.ts (2)
28-29: LGTM!
The new imports are correctly added and necessary for the new test cases.
242-277: 🛠️ Refactor suggestion
Refactor test to improve maintainability.
- Extract common scrolling mock setup into a helper function
- Standardize comment style with exam mode test
- The Course object creation pattern was previously flagged as problematic
+function mockScrollPoint(component: ComplaintsStudentViewComponent): jest.Mock {
+ const scrollIntoViewMock = jest.fn();
+ component.complaintScrollpoint = {
+ nativeElement: {
+ scrollIntoView: scrollIntoViewMock,
+ },
+ } as ElementRef;
+ return scrollIntoViewMock;
+}
it('should set complaint type COMPLAINT and scroll to complaint form when pressing complaint', fakeAsync(() => {
testInitWithResultStub(of());
- const courseWithMaxComplaints: Course = {
- ...course,
- maxComplaints: 3,
- };
- const exerciseWithMaxComplaints: Exercise = {
- ...courseExercise,
- course: courseWithMaxComplaints,
- };
+ const courseWithMaxComplaints = { id: 1, maxComplaints: 3 } as Course;
+ const exerciseWithMaxComplaints = { id: 1, course: courseWithMaxComplaints } as Exercise;
component.course = courseWithMaxComplaints;
component.exercise = exerciseWithMaxComplaints;
component.showSection = true;
component.isCorrectUserToFileAction = true;
component.remainingNumberOfComplaints = 1;
fixture.detectChanges();
- // Mock complaint scrollpoint
- const scrollIntoViewMock = jest.fn();
- component.complaintScrollpoint = {
- nativeElement: {
- scrollIntoView: scrollIntoViewMock,
- },
- } as ElementRef;
+ const scrollIntoViewMock = mockScrollPoint(component);
const button = fixture.debugElement.nativeElement.querySelector('#complain');
button.click();
fixture.detectChanges();
expect(component.formComplaintType).toBe(ComplaintType.COMPLAINT);
- tick(); // Wait for update to happen
+ tick(); // Wait for setTimeout to execute
expect(scrollIntoViewMock).toHaveBeenCalledWith({ behavior: 'smooth' });
}));This issue with Course object creation was previously flagged. Using type assertion is preferred over spread operator with types.
|
Could not deploy for some reason :(( |
|
Any idea why the deployment does not work? |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
Not stale |
badkeyy
changed the title
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Fixes #9735
Description
Sometimes students were not able to see that the complaint form opened after pressing the "Complain" button. With this PR the browser windows gets scrolled down to the end of the form (To after the submit button) after pressing the button.
Steps for Testing
Prerequisites:
Exam Mode 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
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests