Skip to content

refactor: button & card ripple #4284

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.

Sign up for GitHub

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

Merged
jrgarciadev merged 4 commits into from
Dec 10, 2024
Merged

refactor: button & card ripple #4284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
469324b
chore: update use-button.ts
sudongyuer Dec 8, 2024
74d29bf
chore: update use-ripple.ts
sudongyuer Dec 8, 2024
e519216
chore: update use-card.ts
sudongyuer Dec 8, 2024
dce6aca
chore: add changeset
sudongyuer Dec 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/rude-cobras-grab.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@nextui-org/button": patch
"@nextui-org/ripple": patch
"@nextui-org/card": patch
---

Refactor Button & Card Ripple
16 changes: 8 additions & 8 deletions packages/components/button/src/use-button.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {useDOMRef, filterDOMProps} from "@nextui-org/react-utils";
import {button} from "@nextui-org/theme";
import {isValidElement, cloneElement, useMemo} from "react";
import {useAriaButton} from "@nextui-org/use-aria-button";
import {useHover} from "@react-aria/interactions";
import {PressEvent, useHover} from "@react-aria/interactions";
import {SpinnerProps} from "@nextui-org/spinner";
import {useRipple} from "@nextui-org/ripple";

Expand Down Expand Up @@ -135,22 +135,22 @@ export function useButton(props: UseButtonProps) {
],
);

const {onClick: onRippleClickHandler, onClear: onClearRipple, ripples} = useRipple();
const {onPress: onRipplePressHandler, onClear: onClearRipple, ripples} = useRipple();

const handleClick = useCallback(
(e: React.MouseEvent<HTMLButtonElement>) => {
const handlePress = useCallback(
(e: PressEvent) => {
if (disableRipple || isDisabled || disableAnimation) return;
domRef.current && onRippleClickHandler(e);
domRef.current && onRipplePressHandler(e);
},
[disableRipple, isDisabled, disableAnimation, domRef, onRippleClickHandler],
[disableRipple, isDisabled, disableAnimation, domRef, onRipplePressHandler],
);

const {buttonProps: ariaButtonProps, isPressed} = useAriaButton(
{
elementType: as,
isDisabled,
onPress,
onClick: chain(onClick, handleClick),
onPress: chain(onPress, handlePress),
onClick: onClick,
...otherProps,
} as AriaButtonProps,
domRef,
Expand Down
24 changes: 13 additions & 11 deletions packages/components/card/src/use-card.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import type {AriaButtonProps} from "@nextui-org/use-aria-button";
import type {RippleProps} from "@nextui-org/ripple";

import {card} from "@nextui-org/theme";
import {MouseEvent, ReactNode, useCallback, useMemo} from "react";
import {ReactNode, useCallback, useMemo} from "react";
import {chain, mergeProps} from "@react-aria/utils";
import {useFocusRing} from "@react-aria/focus";
import {useHover} from "@react-aria/interactions";
import {PressEvent, useHover} from "@react-aria/interactions";
import {useAriaButton} from "@nextui-org/use-aria-button";
import {
HTMLNextUIProps,
Expand Down Expand Up @@ -96,20 +96,22 @@ export function useCard(originalProps: UseCardProps) {

const baseStyles = clsx(classNames?.base, className);

const {onClick: onRippleClickHandler, onClear: onClearRipple, ripples} = useRipple();
const {onClear: onClearRipple, onPress: onRipplePressHandler, ripples} = useRipple();

const handleClick = (e: MouseEvent<HTMLDivElement>) => {
if (!disableAnimation && !disableRipple && domRef.current) {
onRippleClickHandler(e);
}
};
const handlePress = useCallback(
(e: PressEvent) => {
if (disableRipple || disableAnimation) return;
domRef.current && onRipplePressHandler(e);
},
[disableRipple, disableAnimation, domRef, onRipplePressHandler],
);

const {buttonProps, isPressed} = useAriaButton(
{
onPress,
onPress: chain(onPress, handlePress),
elementType: as,
isDisabled: !originalProps.isPressable,
onClick: chain(onClick, handleClick),
onClick: onClick,
allowTextSelectionOnPress,
...otherProps,
} as unknown as AriaButtonProps<"button">,
Expand Down Expand Up @@ -209,7 +211,7 @@ export function useCard(originalProps: UseCardProps) {
isPressable: originalProps.isPressable,
isHoverable: originalProps.isHoverable,
disableRipple,
handleClick,
handlePress,
isFocusVisible,
getCardProps,
getRippleProps,
Expand Down
12 changes: 6 additions & 6 deletions packages/components/ripple/src/use-ripple.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {getUniqueID} from "@nextui-org/shared-utils";
import React, {useCallback, useState} from "react";
import {PressEvent} from "@react-types/shared";

export type RippleType = {
key: React.Key;
Expand All @@ -13,19 +14,18 @@ export interface UseRippleProps {}
export function useRipple(props: UseRippleProps = {}) {
const [ripples, setRipples] = useState<RippleType[]>([]);

const onClick = useCallback((event: React.MouseEvent<HTMLElement, MouseEvent>) => {
const trigger = event.currentTarget;
const onPress = useCallback((event: PressEvent) => {
const trigger = event.target;
Comment on lines +17 to +18
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider using event.currentTarget instead of event.target

Using event.target might lead to incorrect ripple positioning when clicking on child elements within the button/card. The target could be a nested element, while currentTarget always refers to the element the event handler is attached to.

-  const trigger = event.target;
+  const trigger = event.currentTarget;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const onPress = useCallback((event: PressEvent) => {
const trigger = event.target;
const onPress = useCallback((event: PressEvent) => {
const trigger = event.currentTarget;


const size = Math.max(trigger.clientWidth, trigger.clientHeight);
const rect = trigger.getBoundingClientRect();

setRipples((prevRipples) => [
...prevRipples,
{
key: getUniqueID(prevRipples.length.toString()),
size,
x: event.clientX - rect.left - size / 2,
y: event.clientY - rect.top - size / 2,
x: event.x - size / 2,
y: event.y - size / 2,
Comment on lines +27 to +28
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Scroll position needs to be accounted for in ripple coordinates

The current implementation uses event.x and event.y which are viewport-relative coordinates. This will cause misaligned ripples when the page is scrolled since these coordinates don't account for scroll position. The suggested fix in the review comment using event.pageX, event.pageY and getBoundingClientRect() is the correct approach as it properly handles scroll position.

  • packages/components/ripple/src/use-ripple.ts: Replace viewport coordinates with page-relative coordinates that account for scroll position and element position.
🔗 Analysis chain

Verify scroll position handling in coordinate calculation

The direct use of event.x and event.y might not account for scroll position, potentially causing misaligned ripples when the page is scrolled.

-    x: event.x - size / 2,
-    y: event.y - size / 2,
+    x: event.pageX - trigger.getBoundingClientRect().left - size / 2,
+    y: event.pageY - trigger.getBoundingClientRect().top - size / 2,

Let me gather more context about the ripple implementation to verify the coordinate calculation.

Let me try a different approach to examine the ripple implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for scroll position handling in ripple implementations
rg -A 5 "scroll(Left|Top)" 
ast-grep --pattern 'event.x'

Length of output: 2846

Script:

#!/bin/bash
# Check the complete ripple implementation and event handling
ast-grep --pattern 'function useRipple' -A 30
rg -l "getBoundingClientRect|pageX|pageY" packages/components/ripple/

Length of output: 119

Script:

#!/bin/bash
# Check the complete ripple implementation file content
cat packages/components/ripple/src/use-ripple.ts

# Also check for any test files that might show the expected behavior
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx ripple packages/components/

Length of output: 1169

},
]);
}, []);
Expand All @@ -34,7 +34,7 @@ export function useRipple(props: UseRippleProps = {}) {
setRipples((prevState) => prevState.filter((ripple) => ripple.key !== key));
}, []);

return {ripples, onClick, onClear, ...props};
return {ripples, onClear, onPress, ...props};
}

export type UseRippleReturn = ReturnType<typeof useRipple>;