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. --- src/components/organisms/navbar/index.ts | 1 + .../navbar/navbar-item/navbar-item.stories.tsx | 23 +--- .../navbar/navbar-item/navbar-item.test.tsx | 63 ++++++----- .../organisms/navbar/navbar-item/navbar-item.tsx | 41 +++++--- src/components/organisms/navbar/navbar.module.scss | 2 +- src/components/organisms/navbar/navbar.stories.tsx | 117 ++++++++------------- src/components/organisms/navbar/navbar.test.tsx | 47 +++------ src/components/organisms/navbar/navbar.tsx | 31 +----- 8 files changed, 123 insertions(+), 202 deletions(-) (limited to 'src/components/organisms/navbar') diff --git a/src/components/organisms/navbar/index.ts b/src/components/organisms/navbar/index.ts index f5899d0..afa9cb6 100644 --- a/src/components/organisms/navbar/index.ts +++ b/src/components/organisms/navbar/index.ts @@ -1 +1,2 @@ export * from './navbar'; +export * from './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 ( diff --git a/src/components/organisms/navbar/navbar.module.scss b/src/components/organisms/navbar/navbar.module.scss index 4041825..5af884e 100644 --- a/src/components/organisms/navbar/navbar.module.scss +++ b/src/components/organisms/navbar/navbar.module.scss @@ -47,7 +47,7 @@ } } -.item { +:where(.wrapper) > * { display: flex; justify-content: flex-end; } diff --git a/src/components/organisms/navbar/navbar.stories.tsx b/src/components/organisms/navbar/navbar.stories.tsx index fef995e..95b71ef 100644 --- a/src/components/organisms/navbar/navbar.stories.tsx +++ b/src/components/organisms/navbar/navbar.stories.tsx @@ -1,5 +1,6 @@ import type { ComponentMeta, ComponentStory } from '@storybook/react'; import { Navbar as NavbarComponent } from './navbar'; +import { NavbarItem } from './navbar-item'; /** * Navbar - Storybook Meta @@ -7,28 +8,15 @@ import { Navbar as NavbarComponent } from './navbar'; export default { title: 'Organisms/Navbar', component: NavbarComponent, - args: { - searchPage: '#', - }, argTypes: { - nav: { - description: 'The main nav items.', + children: { + description: 'The navbar items.', type: { name: 'object', required: true, value: {}, }, }, - searchPage: { - control: { - type: 'text', - }, - description: 'The search results page url.', - type: { - name: 'string', - required: true, - }, - }, }, parameters: { layout: 'fullscreen', @@ -39,72 +27,51 @@ const Template: ComponentStory = (args) => ( ); -const doNothing = () => { - // do nothing; +/** + * Navbar Stories - 1 item + */ +export const OneItem = Template.bind({}); +OneItem.args = { + children: ( + + The main nav contents + + ), }; /** - * Navbar Stories - With all items inactive + * Navbar Stories - 2 items */ -export const NavbarInactiveItems = Template.bind({}); -NavbarInactiveItems.args = { - items: [ - { - icon: 'hamburger', - id: 'main-nav', - isActive: false, - label: 'Nav', - contents: 'Main nav contents', - onToggle: doNothing, - }, - { - icon: 'magnifying-glass', - id: 'search', - isActive: false, - label: 'Search', - contents: 'Search contents', - onToggle: doNothing, - }, - { - icon: 'cog', - id: 'settings', - isActive: false, - label: 'Settings', - contents: 'Settings contents', - onToggle: doNothing, - }, - ], +export const TwoItems = Template.bind({}); +TwoItems.args = { + children: ( + <> + + The main nav contents + + + A search form + + + ), }; /** - * Navbar Stories - With one item active + * Navbar Stories - 3 items */ -export const NavbarActiveItem = Template.bind({}); -NavbarActiveItem.args = { - items: [ - { - icon: 'hamburger', - id: 'main-nav', - isActive: true, - label: 'Nav', - contents: 'Main nav contents', - onToggle: doNothing, - }, - { - icon: 'magnifying-glass', - id: 'search', - isActive: false, - label: 'Search', - contents: 'Search contents', - onToggle: doNothing, - }, - { - icon: 'cog', - id: 'settings', - isActive: false, - label: 'Settings', - contents: 'Settings contents', - onToggle: doNothing, - }, - ], +export const ThreeItems = Template.bind({}); +ThreeItems.args = { + children: ( + <> + + The main nav contents + + + A search form + + + A settings form + + + ), }; diff --git a/src/components/organisms/navbar/navbar.test.tsx b/src/components/organisms/navbar/navbar.test.tsx index 35b33f2..6578672 100644 --- a/src/components/organisms/navbar/navbar.test.tsx +++ b/src/components/organisms/navbar/navbar.test.tsx @@ -1,42 +1,21 @@ import { describe, expect, it } from '@jest/globals'; import { render, screen as rtlScreen } from '@testing-library/react'; -import { Navbar, type NavbarItems } from './navbar'; - -const doNothing = () => { - // do nothing; -}; - -const items: NavbarItems = [ - { - icon: 'hamburger', - id: 'main-nav', - isActive: false, - label: 'Nav', - contents: 'Main nav contents', - onToggle: doNothing, - }, - { - icon: 'magnifying-glass', - id: 'search', - isActive: false, - label: 'Search', - contents: 'Search contents', - onToggle: doNothing, - }, - { - icon: 'cog', - id: 'settings', - isActive: false, - label: 'Settings', - contents: 'Settings contents', - onToggle: doNothing, - }, -]; +import { Navbar } from './navbar'; +import { NavbarItem } from './navbar-item'; describe('Navbar', () => { it('renders the given items', () => { - render(); + render( + + + Main nav + + + Search form + + + ); - expect(rtlScreen.getAllByRole('listitem')).toHaveLength(items.length); + expect(rtlScreen.getAllByRole('listitem')).toHaveLength(2); }); }); diff --git a/src/components/organisms/navbar/navbar.tsx b/src/components/organisms/navbar/navbar.tsx index ee379e9..39f3c45 100644 --- a/src/components/organisms/navbar/navbar.tsx +++ b/src/components/organisms/navbar/navbar.tsx @@ -4,26 +4,8 @@ import { type ReactNode, } from 'react'; import { List, type ListProps } from '../../atoms'; -import { NavbarItem, type NavbarItemProps } from './navbar-item'; import styles from './navbar.module.scss'; -export type NavbarItemData = Pick< - NavbarItemProps, - | 'icon' - | 'id' - | 'isActive' - | 'label' - | 'modalHeading' - | 'modalVisibleFrom' - | 'onDeactivate' - | 'onToggle' - | 'showIconOnModal' -> & { - contents: ReactNode; -}; - -export type NavbarItems = [NavbarItemData, NavbarItemData?, NavbarItemData?]; - export type NavbarProps = Omit< ListProps, 'children' | 'hideMarker' | 'isHierarchical' | 'isInline' | 'isOrdered' @@ -34,25 +16,18 @@ export type NavbarProps = Omit< * The number of items should not exceed 3 because of the modal position on * small screens. */ - items: NavbarItems; + children: ReactNode; }; const NavbarWithRef: ForwardRefRenderFunction = ( - { className = '', items, ...props }, + { children, className = '', ...props }, ref ) => { const wrapperClass = `${styles.wrapper} ${className}`; - const navItems = items.filter( - (item): item is NavbarItemData => item !== undefined - ); return ( - {navItems.map(({ contents, ...item }) => ( - - {contents} - - ))} + {children} ); }; -- cgit v1.2.3