-
Notifications
You must be signed in to change notification settings - Fork 14
fix: reset template customisation scroll on screen change #8613
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: main
Are you sure you want to change the base?
fix: reset template customisation scroll on screen change #8613
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughModifies the MultiStepForm component to add scroll position reset functionality triggered on active screen changes. Implements a useEffect hook with DOM manipulation to reset scroll position of the MainPanelBody container and attempt to scroll the app header into view using requestAnimationFrame. Adds a dynamic key prop to force component remount when the active screen changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
|
View your CI Pipeline Execution ↗ for commit 05fa09c
☁️ Nx Cloud last updated this comment at |
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
🤖 Fix all issues with AI agents
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.tsx`:
- Around line 129-130: In the MultiStepForm component remove the debug border
styling so it is not shipped to users: locate the style object or JSX element in
MultiStepForm.tsx that contains "py: 10, border: '2px solid red'" and delete the
border property (or replace it with the intended production styling), then run
the UI to confirm spacing is unchanged; ensure no other leftover debug borders
remain in MultiStepForm or related style objects.
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.tsx (2)
78-78: UnusedcontainerRef- remove or utilize it.The
containerRefis created and assigned to theContainerelement but is never used. TheuseEffectusesdocument.querySelectorinstead. Either leverage the ref directly (preferred) or remove it entirely.♻️ Option 1: Remove unused ref
- import { ReactElement, useEffect, useMemo, useRef, useState } from 'react' + import { ReactElement, useEffect, useMemo, useState } from 'react'const [activeScreen, setActiveScreen] = useState<CustomizationScreen>('language') - const containerRef = useRef<HTMLDivElement>(null)<Container - ref={containerRef} maxWidth="sm"♻️ Option 2: Use ref instead of querySelector (preferred)
If you need a ref for future functionality, consider using it in the scroll logic instead of relying on
document.querySelectorwith test IDs.Also applies to: 120-120
163-173: Key-based remount may be redundant with scroll reset.Adding
key={activeScreen}forces a full remount of theBoxand its children on every screen change. While this is a valid pattern, consider:
- The
useEffectalready handles scroll reset—do you also need the remount?- Remounting destroys any internal state in child screens (form inputs, focus, animations).
- If the child screens are already designed to reset on prop changes, the key is unnecessary overhead.
If the intent is purely visual reset, the scroll
useEffectshould suffice. If it's to reset child component state, thekeyapproach is valid but document the intent with a comment.
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.tsx
Outdated
Show resolved
Hide resolved
|
The latest updates on your projects.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.