-
Notifications
You must be signed in to change notification settings - Fork 357
Development: Fix shifted view when clicking on the form status buttons
#12097
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?
Development: Fix shifted view when clicking on the form status buttons
#12097
Conversation
|
@JTNing Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
Exercises: Fix shifted view when clicking on the form status buttonsDevelopment: Fix shifted view when clicking on the form status buttons
End-to-End (E2E) Test Results SummaryTest Strategy: Two-phase execution
Status: E2E tests encountered an error
|
||||||||||||||||||
|
@JTNing Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results SummaryTest Strategy: Two-phase execution
Status:
|
||||||||||||||||||||||||||||||
…icking-on-the-form-status-buttons
WalkthroughThe scrollToHeadline method was refactored to scroll a dedicated container ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
|
@JTNing Test coverage has been automatically updated in the PR description. |
Phase 1: E2E Tests ✅Status: Phase 1 passed Tests Run: e2e/Login.spec.ts e2e/Logout.spec.ts e2e/SystemHealth.spec.ts Details: Check the Phase 1 Test Report for detailed results. This is an automated comment for Phase 1 of the E2E test execution. |
Phase 2: E2E Tests ✅Status: Phase 2 passed Tests Run: e2e/atlas/ e2e/course/ e2e/exam/ e2e/exercise/ e2e/lecture/ Details: Check the Phase 2 Test Report for detailed results. This is an automated comment for Phase 2 of the E2E test execution. |
End-to-End (E2E) Test Results SummaryTest Strategy: Two-phase execution
Status: All E2E tests passed (both phases) Detailed Results: Check the individual phase reports in the workflow run for test counts, timing, and results. |
…icking-on-the-form-status-buttons
Phase 1: E2E Tests ✅Status: Phase 1 passed Tests Run: e2e/Login.spec.ts e2e/Logout.spec.ts e2e/SystemHealth.spec.ts Details: Check the Phase 1 Test Report for detailed results. This is an automated comment for Phase 1 of the E2E test execution. |
Phase 2: E2E Tests ✅Status: Phase 2 passed Tests Run: e2e/atlas/ e2e/course/ e2e/exam/ e2e/exercise/ e2e/lecture/ Details: Check the Phase 2 Test Report for detailed results. This is an automated comment for Phase 2 of the E2E test execution. |
End-to-End (E2E) Test Results SummaryTest Strategy: Two-phase execution
Status: All E2E tests passed (both phases) Detailed Results: Check the individual phase reports in the workflow run for test counts, timing, and results. |
…icking-on-the-form-status-buttons
|
@JTNing Test coverage has been automatically updated in the PR description. |
…icking-on-the-form-status-buttons
|
@JTNing Test coverage has been automatically updated in the PR description. |
Phase 1: E2E Tests ✅Status: Phase 1 passed Tests Run: e2e/Login.spec.ts e2e/Logout.spec.ts e2e/SystemHealth.spec.ts Details: Check the Phase 1 Test Report for detailed results. This is an automated comment for Phase 1 of the E2E test execution. |
Phase 2: E2E Tests ✅Status: Phase 2 passed Tests Run: e2e/atlas/ e2e/course/ e2e/exam/ e2e/exercise/ e2e/lecture/ Details: Check the Phase 2 Test Report for detailed results. This is an automated comment for Phase 2 of the E2E test execution. |
End-to-End (E2E) Test Results SummaryTest Strategy: Two-phase execution
Status: All E2E tests passed (both phases) Detailed Results: Check the individual phase reports in the workflow run for test counts, timing, and results. |
jerrycai0006
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.
Tested on TS3, works as described.
…icking-on-the-form-status-buttons
|
@JTNing Test coverage has been automatically updated in the PR description. |
Phase 1: E2E Tests ✅Status: Phase 1 passed Tests Run: e2e/Login.spec.ts e2e/Logout.spec.ts e2e/SystemHealth.spec.ts Details: Check the Phase 1 Test Report for detailed results. This is an automated comment for Phase 1 of the E2E test execution. |
SamuelRoettgermann
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.
code review, some smal things
|
|
||
| jest.spyOn(document, 'getElementById').mockImplementation((id: string) => { | ||
| if (id === title) return targetElement; | ||
| if (id === 'course-body-container') return null; |
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.
That if statement is just redundant
|
|
||
| const getElementSpy = jest.spyOn(document, 'getElementById').mockImplementation((id: string) => { | ||
| if (id === 'course-body-container') return containerElement; | ||
| if (id === title) return targetElement; |
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.
That if statement is just redundant
| const offset = navbarHeight + statusBarHeight; | ||
|
|
||
| // Compute the target position within the container: | ||
| const containerRect = container.getBoundingClientRect(); | ||
| const targetRect = target.getBoundingClientRect(); | ||
|
|
||
| const currentScrollTop = container.scrollTop; | ||
| const targetTopWithinContainer = targetRect.top - containerRect.top + currentScrollTop; |
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.
Imo all of these variable names are not descriptive enough. What do target and container refer to, respectively? What does currentScrollTop or targetTopWithinContainer refer to? What does scrollTop even mean?
Even the names target and container, that I didn't even highlight here, are not descriptive at all.
It wasn't used before and you didn't add it, so I assume it must already exist? But what does it describe?
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.
container refers to the element that actually handles scrolling on the page (#course-body-container).
target is the section headline element to scroll to.
scrollTop is a standard DOM property representing how many pixels the scroll container has been scrolled vertically.
The calculation here is converting from viewport coordinates to coordinates within the scroll container.
I'll rename the variables to make them more descriptive.
Phase 2: E2E Tests ✅Status: Phase 2 passed Tests Run: e2e/atlas/ e2e/course/ e2e/exam/ e2e/exercise/ e2e/lecture/ Details: Check the Phase 2 Test Report for detailed results. This is an automated comment for Phase 2 of the E2E test execution. |
End-to-End (E2E) Test Results SummaryTest Strategy: Two-phase execution
Status: All E2E tests passed (both phases) Detailed Results: Check the individual phase reports in the workflow run for test counts, timing, and results. |
…icking-on-the-form-status-buttons
…tor if statements to reduce redundancy
…form-status-buttons' of github.com:ls1intum/Artemis into bugfix/exercises/fix-shifted-view-when-clicking-on-the-form-status-buttons
1716f1d
|
@JTNing Test coverage has been automatically updated in the PR description. |
Phase 1: E2E Tests ✅Status: Phase 1 passed Tests Run: e2e/Login.spec.ts e2e/Logout.spec.ts e2e/SystemHealth.spec.ts Details: Check the Phase 1 Test Report for detailed results. This is an automated comment for Phase 1 of the E2E test execution. |
Phase 2: E2E Tests ✅Status: Phase 2 passed Tests Run: e2e/atlas/ e2e/course/ e2e/exam/ e2e/exercise/ e2e/lecture/ Details: Check the Phase 2 Test Report for detailed results. This is an automated comment for Phase 2 of the E2E test execution. |
End-to-End (E2E) Test Results SummaryTest Strategy: Two-phase execution
Status: All E2E tests passed (both phases) Detailed Results: Check the individual phase reports in the workflow run for test counts, timing, and results. |
Summary
This PR fixed shifted view when clicking on the form status buttons.
Checklist
General
Client
Motivation and Context
Issue: #11210
When clicking on the form status bar (for any exercise), the scroll view gets shifted. When scrolling up the page header is not reachable again. Also, the bottom bar is disaligned, and you see content shining through the lower edge. This issue affects the user experience.
Create._.Test.Course.Junting.Ning.-.Google.Chrome.2026-02-05.16-51-50.mp4
Description
Adjusted the scroll logic in the scrollToHeadline function.
Steps for Testing
Prerequisites:
Expected behavior:
Clicking on a breadcrumb should not shift the view
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
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Client
Last updated: 2026-02-09 15:32:08 UTC
Screenshots
Summary by CodeRabbit
Bug Fixes
Tests