Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
14 changes: 13 additions & 1 deletion src/Dialog/Content/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,20 @@ const Content = React.forwardRef<ContentRef, ContentProps>((props, ref) => {
} = props;

const dialogRef = useRef<{ nativeElement: HTMLElement } & CSSMotionStateRef>(null);

const panelRef = useRef<PanelRef>(null);

// Force remount CSSMotion when visible changes from false to false
// This handles React.Activity scenarios where component state is preserved
const [motionKey, setMotionKey] = React.useState(0);
const prevVisibleRef = useRef(visible);

React.useEffect(() => {
if (!visible && !prevVisibleRef.current) {
setMotionKey(k => k + 1);
}
prevVisibleRef.current = visible;
}, [visible]);

// ============================== Refs ==============================
React.useImperativeHandle(ref, () => ({
...panelRef.current,
Expand Down Expand Up @@ -64,6 +75,7 @@ const Content = React.forwardRef<ContentRef, ContentProps>((props, ref) => {
// ============================= Render =============================
return (
<CSSMotion
key={motionKey}
visible={visible}
onVisibleChanged={onVisibleChanged}
onAppearPrepare={onPrepare}
Expand Down
27 changes: 21 additions & 6 deletions src/Dialog/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => {
const lastOutSideActiveElementRef = useRef<HTMLElement>(null);
const wrapperRef = useRef<HTMLDivElement>(null);
const contentRef = useRef<ContentRef>(null);
const closeTimerRef = useRef<ReturnType<typeof setTimeout>>(null);

const [animatedVisible, setAnimatedVisible] = React.useState(visible);

Expand Down Expand Up @@ -157,22 +158,36 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => {

// ========================= Effect =========================
useEffect(() => {
clearTimeout(closeTimerRef.current);

if (visible) {
setAnimatedVisible(true);
saveLastOutSideActiveElementRef();
} else if (
animatedVisible &&
contentRef.current.enableMotion() &&
!contentRef.current.inMotion()
) {
doClose();
} else if (animatedVisible) {
const hasMotion = contentRef.current?.enableMotion?.();
const inMotion = contentRef.current?.inMotion?.();

if (hasMotion && !inMotion) {
doClose();
} else {
closeTimerRef.current = setTimeout(() => {
if (!visible && animatedVisible) {
doClose();
}
}, 500);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The timeout duration 500 is a magic number. It's better to define it as a constant with a descriptive name (e.g., const DIALOG_CLOSE_FALLBACK_TIMEOUT = 500;) at the top of the component. This improves readability and makes the purpose of the timeout clearer and easier to modify in the future.

}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current logic introduces an unnecessary 500ms delay for closing the dialog when motion is disabled (hasMotion is false). If there's no motion, doClose() should be called immediately. The condition should be updated to close the dialog if motion is disabled OR if it's not currently in motion.

Suggested change
if (hasMotion && !inMotion) {
doClose();
} else {
closeTimerRef.current = setTimeout(() => {
if (!visible && animatedVisible) {
doClose();
}
}, 500);
}
if (!hasMotion || !inMotion) {
doClose();
} else {
closeTimerRef.current = setTimeout(() => {
if (!visible && animatedVisible) {
doClose();
}
}, 500);
}

}

return () => {
clearTimeout(closeTimerRef.current);
};
}, [visible]);

// Remove direct should also check the scroll bar update
useEffect(
() => () => {
clearTimeout(contentTimeoutRef.current);
clearTimeout(closeTimerRef.current);
},
[],
);
Expand Down
Loading