add some comments

sumup-oss · Nov 28, 2024 · 2d49335 · 2d49335
1 parent 9495032
commit 2d49335
Showing 1 changed file with 23 additions and 21 deletions.
diff --git a/packages/circuit-ui/components/Dialog/Dialog.tsx b/packages/circuit-ui/components/Dialog/Dialog.tsx
@@ -68,6 +68,15 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
     ref,
   ) => {
     // TODO add translated default close label
+    if (process.env.NODE_ENV !== 'production') {
+      if (!isSufficientlyLabelled(closeButtonLabel)) {
+        throw new AccessibilityError(
+          'Dialog',
+          'The `closeButtonLabel` prop is missing or invalid.',
+        );
+      }
+    }
+
     const dialogRef = useRef<HTMLDialogElement>(null);
     const lastFocusedElementRef = useRef<HTMLElement | null>(null);
     // not sure if necessary:
@@ -87,7 +96,7 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
       }
     }, [open]);
 
-    const handleDialogClose = () => {
+    const handleDialogClose = useCallback(() => {
       const dialogElement = dialogRef.current;
       if (!dialogElement) {
         return;
@@ -96,47 +105,36 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
       if (lastFocusedElementRef.current) {
         setTimeout(() => lastFocusedElementRef.current?.focus());
       }
+      // restore scroll tp page
       document.documentElement.style.overflowY = 'unset';
-
+      // trigger close animation
       dialogElement.classList.remove(classes.show);
+      // trigger dialog close after animation
       setTimeout(() => {
         if (dialogElement.open) {
           dialogElement.close();
         }
       }, animationDuration);
-    };
+    }, []);
 
     const handleOutsideClick = useCallback(() => {
-      if (open) {
-        if (!isModal) {
-          lastFocusedElementRef.current = null;
-          handleDialogClose();
-        }
+      // modal dialogs outside click is handled by onDialogClick
+      if (open && !isModal) {
+        lastFocusedElementRef.current = null;
+        handleDialogClose();
       }
-    }, [isModal, open]);
+    }, [isModal, open, handleDialogClose]);
 
     useClickOutside(dialogRef, handleOutsideClick, open);
 
-    if (process.env.NODE_ENV !== 'production') {
-      if (!isSufficientlyLabelled(closeButtonLabel)) {
-        throw new AccessibilityError(
-          'Dialog',
-          'The `closeButtonLabel` prop is missing or invalid.',
-        );
-      }
-    }
-
     useEffect(() => {
       const dialogElement = dialogRef.current;
-
       if (!dialogElement) {
         return undefined;
       }
-
       // eslint-disable-next-line @typescript-eslint/ban-ts-comment
       // @ts-ignore The package is bundled incorrectly
       dialogPolyfill.registerDialog(dialogElement);
-
       dialogElement.addEventListener('close', onClose);
 
       return () => {
@@ -158,6 +156,7 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
           if (isModal) {
             dialogElement.showModal();
             if (!hasNativeDialog) {
+              // use the polyfill backdrop
               (dialogElement.nextSibling as HTMLDivElement).classList.add(
                 classes.backdropVisible,
                 classes.backdrop,
@@ -166,7 +165,9 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
           } else {
             dialogElement.show();
           }
+          // trigger show animation
           dialogElement.classList.add(classes.show);
+          // if dialog is modal, disable scroll on page
           if (isModal) {
             document.documentElement.style.overflowY = 'hidden';
           }
@@ -184,6 +185,7 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
 
     const onDialogClick = (e: ClickEvent<HTMLDialogElement>) => {
       if (!hasNativeDialog) {
+        // TODO try to make this work with polyfill
         return;
       }
       if (isModal && e.target === e.currentTarget) {