refactor: clean up onboarding scroll, props, and magic values#4154
Closed
devin-ai-integration[bot] wants to merge 1 commit intoc-branch-2from
Closed
refactor: clean up onboarding scroll, props, and magic values#4154devin-ai-integration[bot] wants to merge 1 commit intoc-branch-2from
devin-ai-integration[bot] wants to merge 1 commit intoc-branch-2from
Conversation
- Move scroll-to-active logic into OnboardingSection via useRef + useEffect (self-contained) - Remove stepId prop, data-step attribute, and querySelector pattern from parent - Extract SCROLL_DELAY_MS and DEFAULT_ICON_SIZE named constants - Remove unused onContinue prop from BeforeLogin - Rename 'triggered' to 'autoSignInCompleted' for clarity Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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
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.
refactor: clean up onboarding scroll, props, and magic values
Summary
Refactors the onboarding improvements from #4153 to reduce fragility and hardcoding while keeping the same UX behavior:
TabContentOnboarding) intoOnboardingSectionitself. Each section now owns auseRefand scrolls itself into view when it becomes active. This eliminates thescrollRef+querySelector('[data-step="..."]')pattern and thestepId/data-stepcoupling between parent and child.SCROLL_DELAY_MS(350) andDEFAULT_ICON_SIZE(14) to replace magic numbers.onContinueprop (was aliased to_onContinueand never used). Renamedtriggered→autoSignInCompletedfor clarity.DEFAULT_ICON_SIZEinstead of repeatingsize: 14on each item; only overrides where needed (X icon at 10).Review & Testing Checklist for Human
OnboardingSectionand fires viauseEffectwhenisActivebecomes true. Verify that smooth scrolling still works correctly when advancing/going back through steps — especially the transition from "upcoming" (component returns null, no DOM) to "active" (component renders, ref attaches, effect fires after render with 350ms delay).OnboardingSection:useRefanduseEffectare called before theif (!status || status === "upcoming") return nullguard. This is required by React's rules of hooks but worth confirming there are no edge-case issues on mount/unmount cycles....restspread pattern infinal.tsx: The"size" in rest ? rest.size : DEFAULT_ICON_SIZEapproach works withas constbut is slightly unconventional — confirm it reads well and TypeScript is happy with it in CI.Notes