Repository navigation
feat(ui): tab icon pop animation and logo tap target - #324
Conversation
Add pop animation to tab bar icons on tap using CSS keyframes. AnimatedTabIcon component triggers scale animation on tab selection with spring easing and prefers-reduced-motion support. Increase logo tap target from ~47px to 71px using invisible padding (18px around the 35px icon) so the easter egg is easier to activate. Files: src/App.tsx, src/index.css, src/icons/Logo.tsx
Deploying wallet-bitcoin with
|
| Latest commit: |
4e865cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f0dc6982.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://feat-tab-animation-logo.wallet-bitcoin.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-tab icon animations: App component gains animatingTab state and a triggerTabAnim callback; tab click handlers invoke it. Icons for Wallet, Apps, and Settings are wrapped with an AnimatedTabIcon component. CSS keyframes and classes provide the scale “pop” animation with reduced-motion support. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
🚥 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 |
Deploying wallet-mutinynet with
|
| Latest commit: |
4e865cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://10d2a4d6.arkade-wallet.pages.dev |
| Branch Preview URL: | https://feat-tab-animation-logo.arkade-wallet.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.css`:
- Around line 199-222: The new CSS block for the tab animation
(.tab-icon-animated, `@keyframes` tab-pop, .tab-anim-pop and the
prefers-reduced-motion media query) is not formatted per project Prettier rules;
run Prettier (or your project's formatting command) on src/index.css and commit
the resulting formatted block so the animation styles match the repository's
styling conventions and clear the CI Prettier warning.
| if (!animating || !ref.current) return | ||
| const el = ref.current | ||
| el.classList.remove(animClass) | ||
| void el.offsetWidth |
There was a problem hiding this comment.
What is this doing? Seems like a no-op
There was a problem hiding this comment.
void el.offsetWidth forces a browser reflow — it's a standard trick to reset CSS animations. Without it, removing and re-adding the animation class in the same frame has no effect because the browser batches DOM changes. Reading offsetWidth forces the browser to recalculate layout between the remove and add, so the animation plays again. Added a comment to clarify.
^ written by Claude
bordalix
left a comment
bordalix
left a comment
There was a problem hiding this comment.
ACK, check @pietro909 comment
|
@sahilc0 pull before doing more changes solved conflicts for you |
| const el = ref.current | ||
| el.classList.remove(animClass) | ||
| void el.offsetWidth | ||
| void el.offsetWidth // Force reflow so removing + re-adding the class triggers the animation |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/App.tsx (1)
40-51: HoistanimClassto module scope to silencereact-hooks/exhaustive-deps.
animClassis defined inside the component but used inuseEffectwithout being listed as a dependency. It's a constant string so behavior is correct, but ESLint'sexhaustive-depsrule will flag it. Moving it outside the component is the idiomatic fix.♻️ Proposed fix
+const ANIM_CLASS = 'tab-anim-pop' + function AnimatedTabIcon({ children, animating }: { children: React.ReactNode; animating: boolean }) { const ref = useRef<HTMLDivElement>(null) - const animClass = 'tab-anim-pop' useEffect(() => { if (!animating || !ref.current) return const el = ref.current - el.classList.remove(animClass) + el.classList.remove(ANIM_CLASS) void el.offsetWidth // Force reflow so removing + re-adding the class triggers the animation - el.classList.add(animClass) - const handleEnd = () => el.classList.remove(animClass) + el.classList.add(ANIM_CLASS) + const handleEnd = () => el.classList.remove(ANIM_CLASS) el.addEventListener('animationend', handleEnd) return () => el.removeEventListener('animationend', handleEnd) }, [animating])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 40 - 51, animClass is defined inside the component but used in the useEffect hook without being listed as a dependency, which triggers react-hooks/exhaustive-deps; move the const animClass = 'tab-anim-pop' out of the component to module scope (above the component declaration) and leave all uses in the effect and component (references to animClass in the useEffect that manipulates ref.current, el.classList, handleEnd, and the animating dependency) unchanged so the string is stable and the linter no longer requires it in the dependency array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/App.tsx`:
- Around line 40-51: animClass is defined inside the component but used in the
useEffect hook without being listed as a dependency, which triggers
react-hooks/exhaustive-deps; move the const animClass = 'tab-anim-pop' out of
the component to module scope (above the component declaration) and leave all
uses in the effect and component (references to animClass in the useEffect that
manipulates ref.current, el.classList, handleEnd, and the animating dependency)
unchanged so the string is stable and the linter no longer requires it in the
dependency array.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.tsxsrc/icons/Logo.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/icons/Logo.tsx
Summary
prefers-reduced-motionfor all animationsFiles changed
src/App.tsx— AnimatedTabIcon component, animation trigger on tab selectionsrc/index.css—@keyframes tab-pop,.tab-icon-animatedwrapper, reduced-motion media querysrc/icons/Logo.tsx— padding 18px / margin -18px for larger tap areaTest plan
Summary by CodeRabbit