From f8f11894e869c53a9bb57518fe548f64d301fabe Mon Sep 17 00:00:00 2001 From: Lucas Bordeau Date: Thu, 10 Apr 2025 16:00:50 +0200 Subject: [PATCH] Introduce generic way to close any open dropdown on page location change (#11504) In this PR we introduce a generic way to close any open dropdown idempotently, with the hook useCloseAnyOpenDropdown. We also introduce a generic hook useExecuteTasksOnAnyLocationChange that is called each time the page location changes. This way we can close any open dropdown when the page location changes, which fixes the original issue of having advanced filter dropdown staying open between page changes. Fixes https://github.com/twentyhq/core-team-issues/issues/659 --- .../effect-components/PageChangeEffect.tsx | 7 ++- .../useExecuteTasksOnAnyLocationChange.ts | 16 ++++++ .../useCloseAnyOpenDropdown.test.tsx | 44 ++++++++++++++++ .../useCloseDropdownFromOutside.test.tsx | 43 ++++++++++++++++ .../dropdown/hooks/useCloseAnyOpenDropdown.ts | 51 +++++++++++++++++++ .../hooks/useCloseDropdownFromOutside.ts | 14 +++++ .../useGoBackToPreviousDropdownFocusId.ts | 1 + ...SetFocusedDropdownIdAndMemorizePrevious.ts | 6 +-- 8 files changed, 176 insertions(+), 6 deletions(-) create mode 100644 packages/twenty-front/src/modules/app/hooks/useExecuteTasksOnAnyLocationChange.ts create mode 100644 packages/twenty-front/src/modules/ui/layout/dropdown/hooks/__tests__/useCloseAnyOpenDropdown.test.tsx create mode 100644 packages/twenty-front/src/modules/ui/layout/dropdown/hooks/__tests__/useCloseDropdownFromOutside.test.tsx create mode 100644 packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useCloseAnyOpenDropdown.ts create mode 100644 packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useCloseDropdownFromOutside.ts diff --git a/packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx b/packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx index 78d128fd3..9e91ce7c9 100644 --- a/packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx +++ b/packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx @@ -13,6 +13,7 @@ import { setSessionId, useEventTracker, } from '@/analytics/hooks/useEventTracker'; +import { useExecuteTasksOnAnyLocationChange } from '@/app/hooks/useExecuteTasksOnAnyLocationChange'; import { useRequestFreshCaptchaToken } from '@/captcha/hooks/useRequestFreshCaptchaToken'; import { isCaptchaScriptLoadedState } from '@/captcha/states/isCaptchaScriptLoadedState'; import { isCaptchaRequiredForPath } from '@/captcha/utils/isCaptchaRequiredForPath'; @@ -52,13 +53,17 @@ export const PageChangeEffect = () => { const resetTableSelections = useResetTableRowSelection(objectNamePlural); + const { executeTasksOnAnyLocationChange } = + useExecuteTasksOnAnyLocationChange(); + useEffect(() => { if (!previousLocation || previousLocation !== location.pathname) { setPreviousLocation(location.pathname); + executeTasksOnAnyLocationChange(); } else { return; } - }, [location, previousLocation]); + }, [location, previousLocation, executeTasksOnAnyLocationChange]); const [searchParams] = useSearchParams(); diff --git a/packages/twenty-front/src/modules/app/hooks/useExecuteTasksOnAnyLocationChange.ts b/packages/twenty-front/src/modules/app/hooks/useExecuteTasksOnAnyLocationChange.ts new file mode 100644 index 000000000..a4f1e54d3 --- /dev/null +++ b/packages/twenty-front/src/modules/app/hooks/useExecuteTasksOnAnyLocationChange.ts @@ -0,0 +1,16 @@ +import { useCloseAnyOpenDropdown } from '@/ui/layout/dropdown/hooks/useCloseAnyOpenDropdown'; + +export const useExecuteTasksOnAnyLocationChange = () => { + const { closeAnyOpenDropdown } = useCloseAnyOpenDropdown(); + + /** + * Be careful to put idempotent tasks here. + * + * Because it might be called multiple times. + */ + const executeTasksOnAnyLocationChange = () => { + closeAnyOpenDropdown(); + }; + + return { executeTasksOnAnyLocationChange }; +}; diff --git a/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/__tests__/useCloseAnyOpenDropdown.test.tsx b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/__tests__/useCloseAnyOpenDropdown.test.tsx new file mode 100644 index 000000000..f51add78e --- /dev/null +++ b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/__tests__/useCloseAnyOpenDropdown.test.tsx @@ -0,0 +1,44 @@ +import { expect } from '@storybook/test'; +import { renderHook } from '@testing-library/react'; +import { act } from 'react'; +import { RecoilRoot } from 'recoil'; + +import { useCloseAnyOpenDropdown } from '@/ui/layout/dropdown/hooks/useCloseAnyOpenDropdown'; +import { useDropdown } from '@/ui/layout/dropdown/hooks/useDropdown'; + +const dropdownId = 'test-dropdown-id'; + +const Wrapper = ({ children }: { children: React.ReactNode }) => { + return {children}; +}; + +describe('useCloseAnyOpenDropdown', () => { + it('should open dropdown and then close it with closeAnyOpenDropdown', async () => { + const { result } = renderHook( + () => { + const { openDropdown, isDropdownOpen } = useDropdown(dropdownId); + + const { closeAnyOpenDropdown } = useCloseAnyOpenDropdown(); + + return { closeAnyOpenDropdown, isDropdownOpen, openDropdown }; + }, + { + wrapper: Wrapper, + }, + ); + + expect(result.current.isDropdownOpen).toBe(false); + + act(() => { + result.current.openDropdown(); + }); + + expect(result.current.isDropdownOpen).toBe(true); + + act(() => { + result.current.closeAnyOpenDropdown(); + }); + + expect(result.current.isDropdownOpen).toBe(false); + }); +}); diff --git a/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/__tests__/useCloseDropdownFromOutside.test.tsx b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/__tests__/useCloseDropdownFromOutside.test.tsx new file mode 100644 index 000000000..01c5c5eb4 --- /dev/null +++ b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/__tests__/useCloseDropdownFromOutside.test.tsx @@ -0,0 +1,43 @@ +import { expect } from '@storybook/test'; +import { renderHook } from '@testing-library/react'; +import { act } from 'react'; +import { RecoilRoot } from 'recoil'; + +import { useCloseDropdownFromOutside } from '@/ui/layout/dropdown/hooks/useCloseDropdownFromOutside'; +import { useDropdown } from '@/ui/layout/dropdown/hooks/useDropdown'; + +const dropdownId = 'test-dropdown-id'; + +const Wrapper = ({ children }: { children: React.ReactNode }) => { + return {children}; +}; + +describe('useCloseDropdownFromOutside', () => { + it('should close open dropdown', async () => { + const { result } = renderHook( + () => { + const { isDropdownOpen, openDropdown } = useDropdown(dropdownId); + const { closeDropdownFromOutside } = useCloseDropdownFromOutside(); + + return { closeDropdownFromOutside, isDropdownOpen, openDropdown }; + }, + { + wrapper: Wrapper, + }, + ); + + expect(result.current.isDropdownOpen).toBe(false); + + act(() => { + result.current.openDropdown(); + }); + + expect(result.current.isDropdownOpen).toBe(true); + + act(() => { + result.current.closeDropdownFromOutside(dropdownId); + }); + + expect(result.current.isDropdownOpen).toBe(false); + }); +}); diff --git a/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useCloseAnyOpenDropdown.ts b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useCloseAnyOpenDropdown.ts new file mode 100644 index 000000000..efe0660f8 --- /dev/null +++ b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useCloseAnyOpenDropdown.ts @@ -0,0 +1,51 @@ +import { useCloseDropdownFromOutside } from '@/ui/layout/dropdown/hooks/useCloseDropdownFromOutside'; +import { activeDropdownFocusIdState } from '@/ui/layout/dropdown/states/activeDropdownFocusIdState'; +import { previousDropdownFocusIdState } from '@/ui/layout/dropdown/states/previousDropdownFocusIdState'; +import { usePreviousHotkeyScope } from '@/ui/utilities/hotkey/hooks/usePreviousHotkeyScope'; +import { useRecoilCallback } from 'recoil'; +import { isDefined } from 'twenty-shared/utils'; + +export const useCloseAnyOpenDropdown = () => { + const { goBackToPreviousHotkeyScope } = usePreviousHotkeyScope(); + + const { closeDropdownFromOutside } = useCloseDropdownFromOutside(); + + const closeAnyOpenDropdown = useRecoilCallback( + ({ snapshot, set }) => + () => { + const previousDropdownFocusId = snapshot + .getLoadable(previousDropdownFocusIdState) + .getValue(); + + const activeDropdownFocusId = snapshot + .getLoadable(activeDropdownFocusIdState) + .getValue(); + + const thereIsNoDropdownOpen = + !isDefined(activeDropdownFocusId) && + !isDefined(previousDropdownFocusId); + + if (thereIsNoDropdownOpen) { + return; + } + + const thereIsOneNestedDropdownOpen = isDefined(previousDropdownFocusId); + + if (isDefined(activeDropdownFocusId)) { + closeDropdownFromOutside(activeDropdownFocusId); + } + + if (thereIsOneNestedDropdownOpen) { + closeDropdownFromOutside(previousDropdownFocusId); + } + + set(previousDropdownFocusIdState, null); + set(activeDropdownFocusIdState, null); + + goBackToPreviousHotkeyScope(); + }, + [closeDropdownFromOutside, goBackToPreviousHotkeyScope], + ); + + return { closeAnyOpenDropdown }; +}; diff --git a/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useCloseDropdownFromOutside.ts b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useCloseDropdownFromOutside.ts new file mode 100644 index 000000000..9deb63ee6 --- /dev/null +++ b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useCloseDropdownFromOutside.ts @@ -0,0 +1,14 @@ +import { isDropdownOpenComponentState } from '@/ui/layout/dropdown/states/isDropdownOpenComponentState'; +import { useRecoilCallback } from 'recoil'; + +export const useCloseDropdownFromOutside = () => { + const closeDropdownFromOutside = useRecoilCallback( + ({ set }) => + (dropdownId: string) => { + set(isDropdownOpenComponentState({ scopeId: dropdownId }), false); + }, + [], + ); + + return { closeDropdownFromOutside }; +}; diff --git a/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useGoBackToPreviousDropdownFocusId.ts b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useGoBackToPreviousDropdownFocusId.ts index 5c336ac6c..157aaf5ad 100644 --- a/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useGoBackToPreviousDropdownFocusId.ts +++ b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useGoBackToPreviousDropdownFocusId.ts @@ -3,6 +3,7 @@ import { useRecoilCallback } from 'recoil'; import { activeDropdownFocusIdState } from '@/ui/layout/dropdown/states/activeDropdownFocusIdState'; import { previousDropdownFocusIdState } from '@/ui/layout/dropdown/states/previousDropdownFocusIdState'; +// TODO: this won't work for more than 1 nested dropdown export const useGoBackToPreviousDropdownFocusId = () => { const goBackToPreviousDropdownFocusId = useRecoilCallback( ({ snapshot, set }) => diff --git a/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useSetFocusedDropdownIdAndMemorizePrevious.ts b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useSetFocusedDropdownIdAndMemorizePrevious.ts index 95b923916..2a3bf9dc8 100644 --- a/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useSetFocusedDropdownIdAndMemorizePrevious.ts +++ b/packages/twenty-front/src/modules/ui/layout/dropdown/hooks/useSetFocusedDropdownIdAndMemorizePrevious.ts @@ -7,10 +7,6 @@ export const useSetActiveDropdownFocusIdAndMemorizePrevious = () => { const setActiveDropdownFocusIdAndMemorizePrevious = useRecoilCallback( ({ snapshot, set }) => (dropdownId: string | null) => { - const focusedDropdownId = snapshot - .getLoadable(activeDropdownFocusIdState) - .getValue(); - const activeDropdownFocusId = snapshot .getLoadable(activeDropdownFocusIdState) .getValue(); @@ -19,7 +15,7 @@ export const useSetActiveDropdownFocusIdAndMemorizePrevious = () => { return; } - set(previousDropdownFocusIdState, focusedDropdownId); + set(previousDropdownFocusIdState, activeDropdownFocusId); set(activeDropdownFocusIdState, dropdownId); }, [],