From c6212f927daf3c928f479afa052e4772216a2d8a Mon Sep 17 00:00:00 2001 From: Armand Philippot Date: Tue, 21 Nov 2023 16:10:20 +0100 Subject: refactor(components): replace items prop in Navbar component * replace `items` prop with `children` prop: it is more readable this way, * handle navbar item state inside NavbarItem component: it avoid using three differents states and their methods to do exactly the same thing * remove useAutofocus hook since we can't use it anymore * add `onActivation` and `activationHandlerDelay` prop to NavbarItem component to be able to focus the search input only when the item is activated (it replicates the functioning of useAutofocus hook) * replace `ref` type in SearchForm component: it does not make sense to use an input ref for a form. Instead I use useImperativeHandle to provide different a focus method to the given ref. --- .../navbar/navbar-item/navbar-item.stories.tsx | 23 ++------ .../navbar/navbar-item/navbar-item.test.tsx | 63 ++++++++++++---------- .../organisms/navbar/navbar-item/navbar-item.tsx | 41 ++++++++------ 3 files changed, 63 insertions(+), 64 deletions(-) (limited to 'src/components/organisms/navbar/navbar-item') diff --git a/src/components/organisms/navbar/navbar-item/navbar-item.stories.tsx b/src/components/organisms/navbar/navbar-item/navbar-item.stories.tsx index 1c56768..93b7281 100644 --- a/src/components/organisms/navbar/navbar-item/navbar-item.stories.tsx +++ b/src/components/organisms/navbar/navbar-item/navbar-item.stories.tsx @@ -1,5 +1,4 @@ import type { ComponentMeta, ComponentStory } from '@storybook/react'; -import { useBoolean } from '../../../../utils/hooks'; import { NavbarItem } from './navbar-item'; /** @@ -11,23 +10,9 @@ export default { argTypes: {}, } as ComponentMeta; -const Template: ComponentStory = ({ - isActive, - onDeactivate, - onToggle, - ...args -}) => { - const { deactivate, state, toggle } = useBoolean(isActive); - - return ( - - ); -}; +const Template: ComponentStory = (args) => ( + +); /** * NavbarItem Stories - Default @@ -37,7 +22,6 @@ Default.args = { children: 'The modal contents.', icon: 'cog', id: 'default', - isActive: false, label: 'Open example', }; @@ -49,7 +33,6 @@ ModalVisibleAfterBreakpoint.args = { children: 'The modal contents.', icon: 'cog', id: 'modal-visible', - isActive: false, label: 'Open example', modalVisibleFrom: 'md', }; diff --git a/src/components/organisms/navbar/navbar-item/navbar-item.test.tsx b/src/components/organisms/navbar/navbar-item/navbar-item.test.tsx index 2e7edea..e531ff6 100644 --- a/src/components/organisms/navbar/navbar-item/navbar-item.test.tsx +++ b/src/components/organisms/navbar/navbar-item/navbar-item.test.tsx @@ -1,24 +1,7 @@ -import { describe, expect, it } from '@jest/globals'; +import { describe, expect, it, jest } from '@jest/globals'; import { render, screen as rtlScreen } from '@testing-library/react'; import { userEvent } from '@testing-library/user-event'; -import { useBoolean } from '../../../../utils/hooks'; -import { NavbarItem, type NavbarItemProps } from './navbar-item'; - -const ControlledNavbarItem = ({ - isActive, - ...props -}: Omit) => { - const { deactivate, state, toggle } = useBoolean(isActive); - - return ( - - ); -}; +import { NavbarItem } from './navbar-item'; describe('NavbarItem', () => { it('renders a labelled checkbox to open/close a modal', async () => { @@ -27,14 +10,9 @@ describe('NavbarItem', () => { const user = userEvent.setup(); render( - + {modal} - + ); // eslint-disable-next-line @typescript-eslint/no-magic-numbers @@ -58,9 +36,9 @@ describe('NavbarItem', () => { const user = userEvent.setup(); render( - + {modal} - + ); // eslint-disable-next-line @typescript-eslint/no-magic-numbers @@ -68,6 +46,8 @@ describe('NavbarItem', () => { const controller = rtlScreen.getByRole('checkbox', { name: label }); + await user.click(controller); + expect(controller).toBeChecked(); if (controller.parentElement) await user.click(controller.parentElement); @@ -76,4 +56,31 @@ describe('NavbarItem', () => { // Since the visibility is declared in CSS we cannot use this assertion. //expect(rtlScreen.getByText(modal)).not.toBeVisible(); }); + + /* eslint-disable max-statements */ + it('accepts an activation handler', async () => { + const handler = jest.fn(); + const user = userEvent.setup(); + const label = 'qui'; + + render( + + Some contents + + ); + + // eslint-disable-next-line @typescript-eslint/no-magic-numbers + expect.assertions(3); + + expect(handler).not.toHaveBeenCalled(); + + await user.click(rtlScreen.getByLabelText(label)); + + /* For some reasons (probably setTimeout) it is called twice but if I use + jest fake timers the test throws `Exceeded timeout`... So I leave it with 2 + for now. */ + expect(handler).toHaveBeenCalledTimes(2); + expect(handler).toHaveBeenCalledWith(true); + }); + /* eslint-enable max-statements */ }); diff --git a/src/components/organisms/navbar/navbar-item/navbar-item.tsx b/src/components/organisms/navbar/navbar-item/navbar-item.tsx index 8ef6ce3..993b613 100644 --- a/src/components/organisms/navbar/navbar-item/navbar-item.tsx +++ b/src/components/organisms/navbar/navbar-item/navbar-item.tsx @@ -6,8 +6,11 @@ import { useRef, } from 'react'; import { + useBoolean, useOnClickOutside, + useOnRouteChange, type useOnClickOutsideHandler, + useTimeout, } from '../../../../utils/hooks'; import { Checkbox, @@ -24,10 +27,16 @@ import { import { Modal } from '../../../molecules'; import styles from './navbar-item.module.scss'; +export type NavbarItemActivationHandler = (isActive: boolean) => void; + export type NavbarItemProps = Omit< ListItemProps, 'children' | 'hideMarker' | 'id' > & { + /** + * Add a delay (in ms) before triggering the `onActivation` handler. + */ + activationHandlerDelay?: number; /** * The modal contents. */ @@ -40,10 +49,6 @@ export type NavbarItemProps = Omit< * The item id. */ id: string; - /** - * Should the modal be visible? - */ - isActive: boolean; /** * An accessible name for the nav item. */ @@ -57,13 +62,9 @@ export type NavbarItemProps = Omit< */ modalVisibleFrom?: 'sm' | 'md'; /** - * A callback function to handle modal deactivation. + * A callback function to handle item activation. */ - onDeactivate?: () => void; - /** - * A callback function to handle modal toggle. - */ - onToggle: () => void; + onActivation?: NavbarItemActivationHandler; /** * Should we add the icon on the modal? * @@ -77,16 +78,15 @@ const NavbarItemWithRef: ForwardRefRenderFunction< NavbarItemProps > = ( { + activationHandlerDelay, children, className = '', icon, id, - isActive, label, modalHeading, modalVisibleFrom, - onDeactivate, - onToggle, + onActivation, showIconOnModal = false, ...props }, @@ -99,6 +99,7 @@ const NavbarItemWithRef: ForwardRefRenderFunction< : '', className, ].join(' '); + const { deactivate, state: isActive, toggle } = useBoolean(false); const labelRef = useRef(null); const checkboxRef = useRef(null); const deactivateItem: useOnClickOutsideHandler = useCallback( @@ -107,12 +108,20 @@ const NavbarItemWithRef: ForwardRefRenderFunction< e.target && checkboxRef.current?.contains(e.target as Node); const isLabel = e.target && labelRef.current?.contains(e.target as Node); - if (onDeactivate && !isCheckbox && !isLabel) onDeactivate(); + if (!isCheckbox && !isLabel) deactivate(); }, - [onDeactivate] + [deactivate] ); const modalRef = useOnClickOutside(deactivateItem); + useOnRouteChange(deactivate, 'end'); + + const handleActivation = useCallback(() => { + if (onActivation) onActivation(isActive); + }, [isActive, onActivation]); + + useTimeout(handleActivation, activationHandlerDelay); + return ( -- cgit v1.2.3