fix(toast): hide static toast when dragging to prevent duplicates#5889
fix(toast): hide static toast when dragging to prevent duplicates#5889hassanzadeh-mj wants to merge 6 commits into
Conversation
|
|
@hassanzadeh-mj is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe PR removes drag functionality from the toast component's public API, simplifying animation logic to depend only on region expansion and position. Concurrently, documentation site UI is updated by replacing RandomBanner with ProBanner in the layout, introducing a new V3ReleaseBanner component in the hero section, and removing a version chip from the navbar. Changes
Sequence Diagram(s)sequenceDiagram
participant OldFlow as Toast (Drag Enabled)
participant NewFlow as Toast (Drag Removed)
note over OldFlow: Region Expansion<br/>+ Drag Events<br/>+ Opacity/Height/ScaleX<br/>calculations
note over NewFlow: Region Expansion Only<br/>Static Constraints<br/>Simplified Animation
OldFlow->>OldFlow: Track drag value,<br/>compute elastic constraints
NewFlow->>NewFlow: Use static region-based<br/>constraints
OldFlow->>OldFlow: Apply drag-dependent<br/>visual effects
NewFlow->>NewFlow: Apply position/region-only<br/>visual effects
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/toast/src/toast.tsx (1)
88-90: Remove redundant ref prop.The
ref={domRef}on line 89 is redundant becausegetToastProps()already includesref: domRefin the returned props object (see line 430 in use-toast.ts). Since both reference the same value, this doesn't cause functional issues, but the explicit ref can be removed for cleaner code.Apply this diff to remove the redundancy:
<Component - ref={domRef} {...toastProps} style={{
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/toast/src/toast.tsx(2 hunks)packages/components/toast/src/use-toast.ts(1 hunks)
🔇 Additional comments (3)
packages/components/toast/src/use-toast.ts (1)
718-718: LGTM! Exposing drag state enables the fix.This change correctly exposes the existing internal
dragstate (defined at line 362) as part of the public API, allowing consuming components to react to drag interactions.packages/components/toast/src/toast.tsx (2)
59-59: LGTM! Accessing drag state from the hook.The destructured
dragproperty is now available from theuseToasthook and will be used to control the static toast visibility during drag interactions.
86-87: Effective fix for the duplicate toast issue.Storing
toastPropsin a variable and conditionally overriding the opacity to 0 during drag correctly hides the static toast element, ensuring only the dragged framer-motion wrapper is visible. This resolves the duplicate toast visibility reported in issue #5883.Also applies to: 91-94
|
To remove the underlying toast, a feature needs to be added to Framer Motion, because when dragging occurs, it takes a snapshot of the toast and displays it. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/components/toast/src/use-toast.ts (1)
456-468: Remove duplicatetoast.keyfrom dependencies.The dependency array includes
toast.keytwice (lines 462 and 466), which is redundant. While this doesn't cause runtime issues, it's unnecessary and should be cleaned up.Apply this diff:
[ slots, classNames, toastProps, hoverProps, toast, - toast.key, opacityValue, isToastExiting, state, toast.key, disableAnimation, ],
🧹 Nitpick comments (1)
packages/components/toast/src/use-toast.ts (1)
659-678: Clean up unnecessary dependencies ingetMotionDivProps.The dependency array includes stable values that don't need to be tracked:
dataAttr(line 672) is an imported utility function with a stable referencesetDrag(line 673) is a state setter guaranteed to be stable by ReactRemoving these will prevent unnecessary callback recreation and align with React best practices.
Apply this diff:
[ total, index, placement, isRegionExpanded, isToastExiting, liftHeight, multiplier, initialHeight, frontHeight, toastVariants, classNames, drag, - dataAttr, - setDrag, shouldCloseToast, slots, toastOffset, maxVisibleToasts, ],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/toast/src/use-toast.ts(2 hunks)
🔇 Additional comments (1)
packages/components/toast/src/use-toast.ts (1)
719-719: Exposingdragstate enables conditional styling in consumer.Exposing the
dragboolean allows the toast consumer (toast.tsx) to apply opacity changes selectively to the dragged toast, which aligns with the PR objective. However, this is only effective if the drag state updates correctly (see concern aboutdragListener: falseat line 650).
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-aria-overlay
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-form-reset
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/use-viewport-size
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/docs/components/marketing/hero/v3-release-banner.tsx (2)
27-35: Remove redundant CSS properties.The gradient text span applies the same properties twice—once via Tailwind utilities in
className(e.g.,bg-clip-text,text-transparent) and again via inlinestyle. This duplication is unnecessary and can cause maintenance issues.Apply this diff to remove the redundant inline styles:
<span className="animate-text-gradient inline-flex bg-clip-text font-medium text-transparent bg-[linear-gradient(90deg,#0485f7_0%,#BA8BF6_50%,#0485f7_100%)] dark:bg-[linear-gradient(90deg,#0485f7_0%,#BA8BF6_50%,#0485f7_100%)]" - style={{ - fontSize: "inherit", - backgroundSize: "200%", - backgroundClip: "text", - WebkitBackgroundClip: "text", - color: "transparent", - }} + style={{ + fontSize: "inherit", + backgroundSize: "200%", + }} >
13-46: Consider adding analytics tracking.The previous announcement Chip in
hero.tsxcaptured PostHog events on press. The new V3ReleaseBanner doesn't track clicks, which may result in lost analytics data for this prominent link.If analytics are needed, add an
onClickhandler:+import {usePostHog} from "posthog-js/react"; + export function V3ReleaseBanner() { + const posthog = usePostHog(); + + const handleClick = () => { + posthog.capture("V3 Release Banner", { + action: "click", + category: "landing-page", + data: releaseInfo.href, + }); + }; + return ( <Chip as="a" ... href={releaseInfo.href} + onClick={handleClick}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/docs/app/layout.tsx(2 hunks)apps/docs/components/marketing/hero/hero.tsx(2 hunks)apps/docs/components/marketing/hero/v3-release-banner.tsx(1 hunks)apps/docs/components/navbar.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-25T17:08:46.283Z
Learnt from: adbjo
Repo: heroui-inc/heroui PR: 5846
File: packages/components/tabs/src/tabs.tsx:156-157
Timestamp: 2025-10-25T17:08:46.283Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes `variant` and `isVertical` to prevent potential side-effects, even though they might appear redundant based on static analysis.
Applied to files:
apps/docs/components/navbar.tsx
🧬 Code graph analysis (2)
apps/docs/app/layout.tsx (1)
apps/docs/components/pro-banner.tsx (1)
ProBanner(13-117)
apps/docs/components/marketing/hero/hero.tsx (1)
apps/docs/components/marketing/hero/v3-release-banner.tsx (1)
V3ReleaseBanner(13-46)
🔇 Additional comments (3)
apps/docs/components/navbar.tsx (1)
188-203: LGTM!Commenting out the promotional version chip is a straightforward UI cleanup. The existing
versionChipat line 187 remains functional.apps/docs/app/layout.tsx (1)
16-16: LGTM!Clean replacement of RandomBanner with ProBanner. The component swap maintains the same layout position and structure.
Also applies to: 86-86
apps/docs/components/marketing/hero/hero.tsx (1)
26-28: LGTM! Clean refactoring to dedicated component.Replacing the inline announcement Chip with
V3ReleaseBannerimproves code organization. Note that the analytics tracking for this banner was addressed in the review ofv3-release-banner.tsx.
- Completely removed drag functionality from toast component
- Removed drag-related state (drag, dragValue)
- Removed drag handlers (onDragStart, onDrag, onDragEnd)
- Removed drag props (drag, dragOverlay, dragConstraints, dragElastic)
- Removed drag-related calculations (opacityValue, shouldCloseToast)
- Removed unused constants (SWIPE_THRESHOLD_X, SWIPE_THRESHOLD_Y)
Why drag was removed:
- Drag functionality was causing opacity issues during drag operations
- The drag ghost image (SVG snapshot) created by Framer Motion was causing visual artifacts
- Users can still close toasts using the close button
If drag functionality is needed in the future, use the official Framer Motion solution:
- Use dragListener={false} to prevent automatic drag listener creation
- Use useDragControls() for manual drag control
- This prevents Framer Motion from creating the SVG/ghost layer snapshot
Fixes #5883
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/toast/src/use-toast.ts (1)
499-523: Clarify the drag restoration guidance to prevent future issues.The comment suggests using
dragListener={false}to prevent snapshot creation, but this is incomplete guidance. As identified in previous reviews, settingdragListener={false}alone disables Framer Motion's automatic pointer handlers, which preventsonDragStart,onDrag, andonDragEndfrom firing.The example should clarify that manual event handling is also required when using
dragListener={false}withdragControls.Apply this diff to improve the documentation:
* If drag functionality is needed in the future, the official Framer Motion solution is: - * - Use `dragListener={false}` to prevent automatic drag listener creation - * - Use `useDragControls()` for manual drag control if needed - * - This prevents Framer Motion from creating the SVG/ghost layer snapshot + * - Use `dragListener={false}` AND `useDragControls()` together + * - Implement manual event handlers (onPointerDown) to call controls.start() + * - This prevents Framer Motion from creating the SVG/ghost layer snapshot + * - Note: dragListener={false} alone will break drag handlers; dragControls are required * * Example: * ```tsx * const controls = useDragControls(); + * + * function handlePointerDown(event) { + * controls.start(event); + * } + * * <motion.div * drag="y" * dragControls={controls} * dragListener={false} // Prevents snapshot creation + * onPointerDown={handlePointerDown} // Manual drag initiation * /> * ```
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/toast/src/use-toast.ts(3 hunks)
🔇 Additional comments (2)
packages/components/toast/src/use-toast.ts (2)
536-566: LGTM! Animation logic correctly simplified.The animation properties now cleanly depend on region expansion state and toast position indices, with all drag-related calculations successfully removed. The logic is straightforward and maintains proper stacking behavior.
598-636: Drag functionality removal verified—no issues found.The verification confirms that drag-related code has been completely and safely removed from the toast component:
- No drag-related references in
toast.tsxUseToastReturntype automatically remains clean (derived from function return)- Close button functionality properly preserved
- No orphaned properties or broken references detected
|
@wingkwong |
wingkwong
left a comment
wingkwong
left a comment
There was a problem hiding this comment.
please don't include unrelated changes in this PR. The upstream should be canary, not main.
- Remove all drag-related state management (drag, dragValue) - Remove drag event handlers (onDrag, onDragEnd, onDragStart) - Remove drag constraints and elastic properties - Remove drag-related utility functions (shouldCloseToast, getDragElasticConstraints) - Remove drag-related constants (SWIPE_THRESHOLD_X, SWIPE_THRESHOLD_Y) - Remove drag-related data attributes (data-drag, data-drag-value) - Remove opacity calculations based on drag values Users can now dismiss toasts only through the close button and timeout, providing a better and clearer user experience. Related to heroui-inc#5889
3 participants
Closes #
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Release Notes
New Features
Refactor