fix: [DHIS2-21286] fix scrolling to first validation error in tracker/event forms#4592
Draft
karolinelien wants to merge 2 commits into
Draft
fix: [DHIS2-21286] fix scrolling to first validation error in tracker/event forms#4592karolinelien wants to merge 2 commits into
karolinelien wants to merge 2 commits into
Conversation
…ector Two bugs caused validation-failure scroll to land in the wrong place: 1. withGotoInterface wrapped its class in forwardRef during the React 18 migration (09ed283). The outer ref then pointed at the inner field component, not GotoFieldInterface, so .goto() silently no-op'd on every D2Form section field. Drop the wrapper - no consumer reads the forwarded ref. 2. Both goto() implementations did scrollIntoView() then post-corrected with window.scroll(0, scrolledY - 48). The guard skipped the correction whenever scrolledY === 0 (e.g. OccurredAt near top of doc), leaving the field flush with the viewport top - and behind the sticky ScopeSelector bar. Use scrollIntoView({block:'start'}) + scrollMarginTop:80px on the wrapper so the browser lands the field 80px below the viewport top regardless of position. AI Assisted Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🚀 Deployed on https://deploy-preview-4592.capture.netlify.dhis2.org |
karolinelien
changed the title
JohanGHole
force-pushed
the
fix/DHIS2-21286-validation-error-scroll-positioning-v2
branch
from
e8991b3 to
85c9e42
Compare
Philip-Larsen-Donnelly
force-pushed
the
fix/DHIS2-21286-validation-error-scroll-positioning-v2
branch
from
85c9e42 to
e8991b3
Compare
|
PR #4592 fixed the single-section case, but multi-section forms (common in event programs) still failed to scroll when the empty mandatory field was not in the first section. Two issues in D2Form: 1. validateFormIncludeSectionFailedFields short-circuited with .every(). A section's isValid() inspects the form-wide field UI state (keyed by formId), so a valid earlier section reports invalid when a later section has the failing field. The short-circuit stopped there and collected that section's getInvalidFields() (empty for it), leaving failedFields empty and no field to scroll to. Evaluate every section with .map(...).every(Boolean) so the section that owns the failing field always contributes it. 2. validateFormScrollToFirstFailedField only tried failedFields[0]. Fields that register no instance (e.g. plugins) at the head of the list would no-op the scroll. Find the first failed field that exposes goto(). Export the D2Form class and add a regression test covering the multi-section case, no-goto fields, all-valid, and hidden sections. AI Assisted Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two bugs that caused the scroll-to-first-error on save to land in the wrong place. Affects every entry point that uses
withSaveHandler(TE registration, enrollment registration, single-event registration, new event in enrollment, edit event) — i.e. both tracker and event programs.withGotoInterfacewas wrapped inforwardRefduring the React 18 migration (09ed283). That redirected the parent's ref fromGotoFieldInterfaceto the inner field component, which doesn't expose agoto()method, soinstance.goto && instance.goto()inD2Form.validateFormScrollToFirstFailedFieldsilently no-op'd for every section field. Drop theforwardRefwrapper — no consumer reads the forwarded ref. This explains the pre-fix asymmetry where save would scroll to OccurredAt (which usesDataEntryField's owngoto()) but not to a section mandatory DE.ScopeSelectorbar. Bothgoto()implementations didscrollIntoView()and then post-corrected withwindow.scroll(0, scrolledY - 48). Theif (scrolledY)guard skipped the correction whenscrolledY === 0(which is exactly what happens when the field is near the top of the document), leaving the field flush with the viewport top — under the stickyScopeSelector. Replace withscrollIntoView({ block: 'start' })+scrollMarginTop: '80px'on the wrapper so the browser lands the field 80px below the viewport top regardless of position. 48px covers theSelectorBarheight, the remaining 32px is breathing room above the label.Same fix pattern applied to both call sites (
withGotoInterfacefor in-section data elements andDataEntryFieldfor top-of-form fields).Test plan
Save is always at the bottom of the form, so the only scroll direction validation needs to drive is up.
ScopeSelector(tests B2 via DataEntryField)ScopeSelector(tests B1 — broken on master)ScopeSelectorScopeSelector(tests B1 on the event-program path)AI Assisted