From 1ee4cfd1b79a656f058cbccde237d10dc229ab37 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 27 May 2024 17:45:21 +0200 Subject: [PATCH] [internal] Move `enabled` parameter in hooks to first argument (#3245) * move `enabled` parameter in hooks to front Whenever a hook requires an `enabled` state, the `enabled` parameter is moved to the front. Initially this was the last argument and enabled by default but everywhere that we use these hooks we have to pass a dedicated boolean anyway. This makes sure these hooks follow a similar pattern. Bonus points because Prettier can now improve formatting the usage of these hooks. The reason why is because there is no additional argument after the potential last callback. Before: ```ts let enabled = data.__demoMode ? false : modal && data.comboboxState === ComboboxState.Open useInertOthers( { allowed: useEvent(() => [ data.inputRef.current, data.buttonRef.current, data.optionsRef.current, ]), }, enabled ) ``` After: ```ts let enabled = data.__demoMode ? false : modal && data.comboboxState === ComboboxState.Open useInertOthers(enabled, { allowed: useEvent(() => [ data.inputRef.current, data.buttonRef.current, data.optionsRef.current, ]), }) ``` Much better! * inline variables --- .../src/components/combobox/combobox.tsx | 40 ++++++++-------- .../src/components/dialog/dialog.tsx | 47 ++++++++----------- .../src/components/listbox/listbox.tsx | 42 ++++++++--------- .../src/components/menu/menu.tsx | 43 ++++++++--------- .../src/components/popover/popover.tsx | 26 +++++----- .../src/components/transition/transition.tsx | 2 +- .../src/hooks/use-did-element-move.ts | 5 +- .../src/hooks/use-inert-others.test.tsx | 13 +++-- .../src/hooks/use-inert-others.tsx | 4 +- .../src/hooks/use-on-disappear.ts | 4 +- .../src/hooks/use-outside-click.ts | 4 +- .../src/hooks/use-scroll-lock.ts | 2 +- .../src/hooks/use-tree-walker.ts | 23 ++++----- 13 files changed, 120 insertions(+), 135 deletions(-) diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index b5828b2..de477fd 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -770,10 +770,9 @@ function ComboboxFn actions.closeCombobox(), - data.comboboxState === ComboboxState.Open + let outsideClickEnabled = data.comboboxState === ComboboxState.Open + useOutsideClick(outsideClickEnabled, [data.buttonRef, data.inputRef, data.optionsRef], () => + actions.closeCombobox() ) let slot = useMemo(() => { @@ -1623,25 +1622,25 @@ function OptionsFn( })() // Ensure we close the combobox as soon as the input becomes hidden - useOnDisappear(data.inputRef, actions.closeCombobox, visible) + useOnDisappear(visible, data.inputRef, actions.closeCombobox) // Enable scroll locking when the combobox is visible, and `modal` is enabled - useScrollLock( - ownerDocument, - data.__demoMode ? false : modal && data.comboboxState === ComboboxState.Open - ) + let scrollLockEnabled = data.__demoMode + ? false + : modal && data.comboboxState === ComboboxState.Open + useScrollLock(scrollLockEnabled, ownerDocument) // Mark other elements as inert when the combobox is visible, and `modal` is enabled - useInertOthers( - { - allowed: useEvent(() => [ - data.inputRef.current, - data.buttonRef.current, - data.optionsRef.current, - ]), - }, - data.__demoMode ? false : modal && data.comboboxState === ComboboxState.Open - ) + let inertOthersEnabled = data.__demoMode + ? false + : modal && data.comboboxState === ComboboxState.Open + useInertOthers(inertOthersEnabled, { + allowed: useEvent(() => [ + data.inputRef.current, + data.buttonRef.current, + data.optionsRef.current, + ]), + }) useIsoMorphicEffect(() => { data.optionsPropsRef.current.static = props.static ?? false @@ -1650,9 +1649,8 @@ function OptionsFn( data.optionsPropsRef.current.hold = hold }, [data.optionsPropsRef, hold]) - useTreeWalker({ + useTreeWalker(data.comboboxState === ComboboxState.Open, { container: data.optionsRef.current, - enabled: data.comboboxState === ComboboxState.Open, accept(node) { if (node.getAttribute('role') === 'option') return NodeFilter.FILTER_REJECT if (node.hasAttribute('role')) return NodeFilter.FILTER_SKIP diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx index f940253..8e656ff 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.tsx @@ -260,28 +260,25 @@ function DialogFn( usesOpenClosedState !== null ? (usesOpenClosedState & State.Closing) === State.Closing : false // Ensure other elements can't be interacted with - let inertEnabled = (() => { + let inertOthersEnabled = (() => { + if (__demoMode) return false // Only the top-most dialog should be allowed, all others should be inert if (hasNestedDialogs) return false if (isClosing) return false return enabled })() - - useInertOthers( - { - allowed: useEvent(() => [ - // Allow the headlessui-portal of the Dialog to be interactive. This - // contains the current dialog and the necessary focus guard elements. - internalDialogRef.current?.closest('[data-headlessui-portal]') ?? null, - ]), - disallowed: useEvent(() => [ - // Disallow the "main" tree root node - mainTreeNodeRef.current?.closest('body > *:not(#headlessui-portal-root)') ?? - null, - ]), - }, - __demoMode ? false : inertEnabled - ) + useInertOthers(inertOthersEnabled, { + allowed: useEvent(() => [ + // Allow the headlessui-portal of the Dialog to be interactive. This + // contains the current dialog and the necessary focus guard elements. + internalDialogRef.current?.closest('[data-headlessui-portal]') ?? null, + ]), + disallowed: useEvent(() => [ + // Disallow the "main" tree root node + mainTreeNodeRef.current?.closest('body > *:not(#headlessui-portal-root)') ?? + null, + ]), + }) // Close Dialog on outside click let outsideClickEnabled = (() => { @@ -289,14 +286,10 @@ function DialogFn( if (hasNestedDialogs) return false return true })() - useOutsideClick( - resolveRootContainers, - (event) => { - event.preventDefault() - close() - }, - outsideClickEnabled - ) + useOutsideClick(outsideClickEnabled, resolveRootContainers, (event) => { + event.preventDefault() + close() + }) // Handle `Escape` to close let escapeToCloseEnabled = (() => { @@ -335,10 +328,10 @@ function DialogFn( if (hasParentDialog) return false return true })() - useScrollLock(ownerDocument, __demoMode ? false : scrollLockEnabled, resolveRootContainers) + useScrollLock(scrollLockEnabled, ownerDocument, resolveRootContainers) // Ensure we close the dialog as soon as the dialog itself becomes hidden - useOnDisappear(internalDialogRef, close, dialogState === DialogStates.Open) + useOnDisappear(enabled, internalDialogRef, close) let [describedby, DescriptionProvider] = useDescriptions() diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index 9f9ac41..467f0f4 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.tsx @@ -560,18 +560,15 @@ function ListboxFn< }, [data]) // Handle outside click - useOutsideClick( - [data.buttonRef, data.optionsRef], - (event, target) => { - dispatch({ type: ActionTypes.CloseListbox }) + let outsideClickEnabled = data.listboxState === ListboxStates.Open + useOutsideClick(outsideClickEnabled, [data.buttonRef, data.optionsRef], (event, target) => { + dispatch({ type: ActionTypes.CloseListbox }) - if (!isFocusableElement(target, FocusableMode.Loose)) { - event.preventDefault() - data.buttonRef.current?.focus() - } - }, - data.listboxState === ListboxStates.Open - ) + if (!isFocusableElement(target, FocusableMode.Loose)) { + event.preventDefault() + data.buttonRef.current?.focus() + } + }) let slot = useMemo(() => { return { @@ -927,19 +924,21 @@ function OptionsFn( })() // Ensure we close the listbox as soon as the button becomes hidden - useOnDisappear(data.buttonRef, actions.closeListbox, visible) + useOnDisappear(visible, data.buttonRef, actions.closeListbox) // Enable scroll locking when the listbox is visible, and `modal` is enabled - useScrollLock( - ownerDocument, - data.__demoMode ? false : modal && data.listboxState === ListboxStates.Open - ) + let scrollLockEnabled = data.__demoMode + ? false + : modal && data.listboxState === ListboxStates.Open + useScrollLock(scrollLockEnabled, ownerDocument) // Mark other elements as inert when the listbox is visible, and `modal` is enabled - useInertOthers( - { allowed: useEvent(() => [data.buttonRef.current, data.optionsRef.current]) }, - data.__demoMode ? false : modal && data.listboxState === ListboxStates.Open - ) + let inertOthersEnabled = data.__demoMode + ? false + : modal && data.listboxState === ListboxStates.Open + useInertOthers(inertOthersEnabled, { + allowed: useEvent(() => [data.buttonRef.current, data.optionsRef.current]), + }) let initialOption = useRef(null) @@ -970,7 +969,8 @@ function OptionsFn( // // This can be solved by only transitioning the `opacity` instead of everything, but if you _do_ // want to transition the y-axis for example you will run into the same issue again. - let didButtonMove = useDidElementMove(data.buttonRef, data.listboxState !== ListboxStates.Open) + let didElementMoveEnabled = data.listboxState !== ListboxStates.Open + let didButtonMove = useDidElementMove(didElementMoveEnabled, data.buttonRef) // Now that we know that the button did move or not, we can either disable the panel and all of // its transitions, or rely on the `visible` state to hide the panel whenever necessary. diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index 7320189..1ab43ab 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.tsx @@ -388,18 +388,15 @@ function MenuFn( let menuRef = useSyncRefs(ref) // Handle outside click - useOutsideClick( - [buttonRef, itemsRef], - (event, target) => { - dispatch({ type: ActionTypes.CloseMenu }) + let outsideClickEnabled = menuState === MenuStates.Open + useOutsideClick(outsideClickEnabled, [buttonRef, itemsRef], (event, target) => { + dispatch({ type: ActionTypes.CloseMenu }) - if (!isFocusableElement(target, FocusableMode.Loose)) { - event.preventDefault() - buttonRef.current?.focus() - } - }, - menuState === MenuStates.Open - ) + if (!isFocusableElement(target, FocusableMode.Loose)) { + event.preventDefault() + buttonRef.current?.focus() + } + }) let close = useEvent(() => { dispatch({ type: ActionTypes.CloseMenu }) @@ -624,19 +621,19 @@ function ItemsFn( })() // Ensure we close the menu as soon as the button becomes hidden - useOnDisappear(state.buttonRef, () => dispatch({ type: ActionTypes.CloseMenu }), visible) + useOnDisappear(visible, state.buttonRef, () => { + dispatch({ type: ActionTypes.CloseMenu }) + }) // Enable scroll locking when the menu is visible, and `modal` is enabled - useScrollLock( - ownerDocument, - state.__demoMode ? false : modal && state.menuState === MenuStates.Open - ) + let scrollLockEnabled = state.__demoMode ? false : modal && state.menuState === MenuStates.Open + useScrollLock(scrollLockEnabled, ownerDocument) // Mark other elements as inert when the menu is visible, and `modal` is enabled - useInertOthers( - { allowed: useEvent(() => [state.buttonRef.current, state.itemsRef.current]) }, - state.__demoMode ? false : modal && state.menuState === MenuStates.Open - ) + let inertOthersEnabled = state.__demoMode ? false : modal && state.menuState === MenuStates.Open + useInertOthers(inertOthersEnabled, { + allowed: useEvent(() => [state.buttonRef.current, state.itemsRef.current]), + }) // We keep track whether the button moved or not, we only check this when the menu state becomes // closed. If the button moved, then we want to cancel pending transitions to prevent that the @@ -647,7 +644,8 @@ function ItemsFn( // // This can be solved by only transitioning the `opacity` instead of everything, but if you _do_ // want to transition the y-axis for example you will run into the same issue again. - let didButtonMove = useDidElementMove(state.buttonRef, state.menuState !== MenuStates.Open) + let didButtonMoveEnabled = state.menuState !== MenuStates.Open + let didButtonMove = useDidElementMove(didButtonMoveEnabled, state.buttonRef) // Now that we know that the button did move or not, we can either disable the panel and all of // its transitions, or rely on the `visible` state to hide the panel whenever necessary. @@ -662,9 +660,8 @@ function ItemsFn( container.focus({ preventScroll: true }) }, [state.menuState, state.itemsRef, ownerDocument, state.itemsRef.current]) - useTreeWalker({ + useTreeWalker(state.menuState === MenuStates.Open, { container: state.itemsRef.current, - enabled: state.menuState === MenuStates.Open, accept(node) { if (node.getAttribute('role') === 'menuitem') return NodeFilter.FILTER_REJECT if (node.hasAttribute('role')) return NodeFilter.FILTER_SKIP diff --git a/packages/@headlessui-react/src/components/popover/popover.tsx b/packages/@headlessui-react/src/components/popover/popover.tsx index 823b044..ca5876d 100644 --- a/packages/@headlessui-react/src/components/popover/popover.tsx +++ b/packages/@headlessui-react/src/components/popover/popover.tsx @@ -365,18 +365,15 @@ function PopoverFn( ) // Handle outside click - useOutsideClick( - root.resolveContainers, - (event, target) => { - dispatch({ type: ActionTypes.ClosePopover }) + let outsideClickEnabled = popoverState === PopoverStates.Open + useOutsideClick(outsideClickEnabled, root.resolveContainers, (event, target) => { + dispatch({ type: ActionTypes.ClosePopover }) - if (!isFocusableElement(target, FocusableMode.Loose)) { - event.preventDefault() - button?.focus() - } - }, - popoverState === PopoverStates.Open - ) + if (!isFocusableElement(target, FocusableMode.Loose)) { + event.preventDefault() + button?.focus() + } + }) let close = useEvent( ( @@ -868,10 +865,13 @@ function PanelFn( })() // Ensure we close the popover as soon as the button becomes hidden - useOnDisappear(state.button, () => dispatch({ type: ActionTypes.ClosePopover }), visible) + useOnDisappear(visible, state.button, () => { + dispatch({ type: ActionTypes.ClosePopover }) + }) // Enable scroll locking when the popover is visible, and `modal` is enabled - useScrollLock(ownerDocument, state.__demoMode ? false : modal && visible) + let scrollLockEnabled = state.__demoMode ? false : modal && visible + useScrollLock(scrollLockEnabled, ownerDocument) let handleKeyDown = useEvent((event: ReactKeyboardEvent) => { switch (event.key) { diff --git a/packages/@headlessui-react/src/components/transition/transition.tsx b/packages/@headlessui-react/src/components/transition/transition.tsx index fb35803..d2ddadf 100644 --- a/packages/@headlessui-react/src/components/transition/transition.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.tsx @@ -586,7 +586,7 @@ function TransitionRootFn