From a8d7630ba7905d80745ef60264a6780eabfa970c Mon Sep 17 00:00:00 2001 From: aphilibeaux Date: Mon, 17 Nov 2025 12:16:33 +0100 Subject: [PATCH] fix(popup): handle auto scroll and remove useless deps effect --- biome.json | 6 +- .../components/DateInput/components/Popup.tsx | 4 +- .../ui/src/components/LineChart/helpers.ts | 2 +- .../ui/src/components/Menu/MenuContent.tsx | 2 +- .../components/Modal/components/Dialog.tsx | 2 +- packages/ui/src/components/Popup/index.tsx | 42 +++--- .../SelectInput/components/Dropdown.tsx | 136 ++++++++++++------ packages/ui/src/components/TagInput/index.tsx | 6 +- packages/ui/src/components/TagList/index.tsx | 2 +- .../src/components/VerificationCode/index.tsx | 2 +- pnpm-lock.yaml | 16 +-- 11 files changed, 134 insertions(+), 86 deletions(-) diff --git a/biome.json b/biome.json index 9423b557b7..010ec9c1ac 100644 --- a/biome.json +++ b/biome.json @@ -14,6 +14,7 @@ ".oxlintrc.json", "**/*.ts", "**/*.tsx", + "!.next/**", "!**/dist/**", "!**/build/**", "!**/.turbo/**", @@ -38,8 +39,9 @@ "files": { "includes": [ "**", - "!**/.turbo/", - "!**/.next/", + "!.turbo/*", + "!**/.turbo/*", + "!**/.next/*", "!**/coverage", "!**/node_modules/", "!**/storybook-static", diff --git a/packages/ui/src/components/DateInput/components/Popup.tsx b/packages/ui/src/components/DateInput/components/Popup.tsx index 9e346f2bd9..4750ba6224 100644 --- a/packages/ui/src/components/DateInput/components/Popup.tsx +++ b/packages/ui/src/components/DateInput/components/Popup.tsx @@ -1,7 +1,7 @@ 'use client' import type { Dispatch, ReactNode, RefObject, SetStateAction } from 'react' -import { useEffect, useRef } from 'react' +import { useLayoutEffect, useRef } from 'react' import { Popup } from '../../Popup' import { POPUP_WIDTH } from '../constants' import { dateinputPopup } from './styles.css' @@ -38,7 +38,7 @@ export const CalendarPopup = ({ }: PopupProps) => { const ref = useRef(null) - useEffect(() => { + useLayoutEffect(() => { document.addEventListener('mousedown', event => handleClickOutside(event, ref, setVisible, refInput), ) diff --git a/packages/ui/src/components/LineChart/helpers.ts b/packages/ui/src/components/LineChart/helpers.ts index 4a1daf06b4..92253664a6 100644 --- a/packages/ui/src/components/LineChart/helpers.ts +++ b/packages/ui/src/components/LineChart/helpers.ts @@ -54,7 +54,7 @@ export const getMinChartValue = (preppedData?: Serie[]): number => { } export const getCurrent = (values: number[] = []): number => - values.length > 0 ? values[values.length - 1] : 0 + values.length > 0 ? values.at(-1) : 0 export const getSelected = ( label: string, diff --git a/packages/ui/src/components/Menu/MenuContent.tsx b/packages/ui/src/components/Menu/MenuContent.tsx index eb3f86f96a..e2390b64fe 100644 --- a/packages/ui/src/components/Menu/MenuContent.tsx +++ b/packages/ui/src/components/Menu/MenuContent.tsx @@ -198,7 +198,7 @@ export const Menu = forwardRef( if (indexOfCurrent > 0) { listItem[indexOfCurrent - 1].focus() } else { - listItem[listItem.length - 1].focus() + listItem.at(-1).focus() } } else if (event.key === 'ArrowLeft' && triggerMethod === 'hover') { disclosureRef.current?.focus() diff --git a/packages/ui/src/components/Modal/components/Dialog.tsx b/packages/ui/src/components/Modal/components/Dialog.tsx index 7771f0dc3b..237f309bfd 100644 --- a/packages/ui/src/components/Modal/components/Dialog.tsx +++ b/packages/ui/src/components/Modal/components/Dialog.tsx @@ -172,7 +172,7 @@ export const Dialog = ({ } const firstFocusableEl = focusableEls[0] as HTMLElement - const lastFocusableEl = focusableEls[focusableEls.length - 1] as HTMLElement + const lastFocusableEl = focusableEls.at(-1) as HTMLElement if (event.shiftKey) { if ( diff --git a/packages/ui/src/components/Popup/index.tsx b/packages/ui/src/components/Popup/index.tsx index 480fa74793..78d3907cff 100644 --- a/packages/ui/src/components/Popup/index.tsx +++ b/packages/ui/src/components/Popup/index.tsx @@ -17,6 +17,7 @@ import { useEffect, useId, useImperativeHandle, + useLayoutEffect, useMemo, useRef, useState, @@ -58,7 +59,7 @@ export type PositionsType = { } /** - * This event handle allow us to not bubble the event to document.body like this react-select works fine + * This event handle allow us to not bubble the event to document.body like this select works fine */ const stopClickPropagation: MouseEventHandler = event => { event.nativeEvent.stopImmediatePropagation() @@ -182,6 +183,7 @@ export const Popup = forwardRef( useImperativeHandle(ref, () => innerPopupRef.current as HTMLDivElement) const timer = useRef>(undefined) + const popupPortalTarget = useMemo(() => { if (portalTarget) { return portalTarget @@ -204,8 +206,7 @@ export const Popup = forwardRef( } return null - // oxlint-disable react/exhaustive-deps - }, [portalTarget, role, childrenRef.current]) + }, [portalTarget, role]) // There are some issue when mixing animation and maxHeight on some browsers, so we disable animation if maxHeight is set. const animationDuration = @@ -227,17 +228,18 @@ export const Popup = forwardRef( ) const generatePopupPositions = useCallback(() => { - if (childrenRef.current && innerPopupRef.current) { - setPositions( - computePositions({ - align, - childrenRef, - hasArrow, - placement, - popupPortalTarget: popupPortalTarget as HTMLElement, - popupRef: innerPopupRef, - }), - ) + console.debug('generatePopupPositions') + if (childrenRef.current && innerPopupRef.current && popupPortalTarget) { + const position = computePositions({ + align, + childrenRef, + hasArrow, + placement, + popupPortalTarget, + popupRef: innerPopupRef, + }) + setPositions(prevPosition => ({ ...prevPosition, ...position })) + console.debug({ ...position }) } }, [hasArrow, placement, popupPortalTarget, align]) @@ -350,7 +352,7 @@ export const Popup = forwardRef( * Once popup is visible in the dom we can compute positions, then set it visible on screen and add event to * recompute positions on scroll or screen resize. */ - useEffect(() => { + useLayoutEffect(() => { if (visibleInDom) { generatePopupPositions() @@ -379,12 +381,11 @@ export const Popup = forwardRef( ]) // This will be triggered when positions are computed and popup is visible in the dom. - useEffect(() => { + useLayoutEffect(() => { if (visibleInDom && innerPopupRef.current) { innerPopupRef.current.style.opacity = '1' } - // oxlint-disable react/exhaustive-deps - }, [positions]) + }, [positions, visibleInDom]) /** * If popup has `visible` prop it means the popup is manually controlled through this prop. @@ -439,6 +440,7 @@ export const Popup = forwardRef( visibleInDom, innerPopupRef, childrenRef, + id, hideOnClickOutside, ]) @@ -463,9 +465,7 @@ export const Popup = forwardRef( } const firstFocusableEl = focusableEls[0] as HTMLElement - const lastFocusableEl = focusableEls[ - focusableEls.length - 1 - ] as HTMLElement + const lastFocusableEl = focusableEls.at(-1) as HTMLElement if (event.shiftKey) { if ( diff --git a/packages/ui/src/components/SelectInput/components/Dropdown.tsx b/packages/ui/src/components/SelectInput/components/Dropdown.tsx index 76c2a58cc9..b885122132 100644 --- a/packages/ui/src/components/SelectInput/components/Dropdown.tsx +++ b/packages/ui/src/components/SelectInput/components/Dropdown.tsx @@ -2,17 +2,20 @@ import { useTheme } from '@ultraviolet/themes' import type { + ChangeEvent, ComponentProps, Dispatch, KeyboardEvent, + MouseEvent, ReactNode, RefObject, SetStateAction, } from 'react' import { + use, useCallback, - useContext, useEffect, + useLayoutEffect, useMemo, useRef, useState, @@ -219,13 +222,26 @@ const CreateDropdown = ({ ) } - const handleClick = (clickedOption: OptionType, group?: string) => { + const handleClick = ({ + clickedOption, + group, + event, + }: { + clickedOption: OptionType + group?: string + event: + | MouseEvent + | KeyboardEvent + | ChangeEvent + }) => { + event.stopPropagation() + setSelectedData({ clickedOption, group, type: 'selectOption' }) if (multiselect) { if (selectedData.selectedValues.includes(clickedOption.value)) { onChange?.( selectedData.selectedValues.filter( - val => val !== clickedOption.value, + value => value !== clickedOption.value, ), ) } else { @@ -420,16 +436,25 @@ const CreateDropdown = ({ data-testid={`option-${option.value}`} id={`option-${indexOption}`} key={option.value} - onClick={() => { + onClick={event => { if (!option.disabled) { - handleClick(option, group) + handleClick({ + clickedOption: option, + event, + group, + }) + } + }} + onKeyDown={event => { + const shouldClick = [' ', 'Enter'].includes(event.key) + if (shouldClick) { + handleClick({ + clickedOption: option, + event, + group, + }) } }} - onKeyDown={event => - [' ', 'Enter'].includes(event.key) - ? handleClick(option, group) - : null - } ref={ option.value === defaultSearchValue || option.searchText === defaultSearchValue @@ -548,14 +573,23 @@ const CreateDropdown = ({ data-testid={`option-${option.value}`} id={`option-${index}`} key={option.value} - onClick={() => { + onClick={event => { if (!option.disabled) { - handleClick(option) + handleClick({ + clickedOption: option, + event, + }) + } + }} + onKeyDown={event => { + const shouldClick = [' ', 'Enter'].includes(event.key) + if (shouldClick) { + handleClick({ + clickedOption: option, + event, + }) } }} - onKeyDown={event => - [' ', 'Enter'].includes(event.key) ? handleClick(option) : null - } ref={ option.value === defaultSearchValue || option.searchText === defaultSearchValue @@ -573,9 +607,12 @@ const CreateDropdown = ({ } className={dropdownCheckbox} disabled={option.disabled} - onChange={() => { + onChange={event => { if (!option.disabled) { - handleClick(option) + handleClick({ + clickedOption: option, + event, + }) } }} tabIndex={-1} @@ -637,9 +674,9 @@ export const Dropdown = ({ const [maxWidth, setWidth] = useState( refSelect.current?.offsetWidth ?? '100%', ) - const modalContext = useContext(ModalContext) + const modalContext = use(ModalContext) - useEffect(() => { + useLayoutEffect(() => { if (refSelect.current && isDropdownVisible) { const position = refSelect.current.getBoundingClientRect().bottom + @@ -647,25 +684,43 @@ export const Dropdown = ({ Number(theme.sizing[INPUT_SIZE_HEIGHT[size]].replace('rem', '')) * 16 + Number.parseInt(theme.space['5'], 10) const overflow = position - window.innerHeight + 32 + if (overflow > 0 && modalContext) { const currentModal = modalContext.openedModals[0] const modalElement = currentModal?.ref.current if (modalElement) { - const parentElement = modalElement.parentNode as HTMLElement - if (parentElement) { + const parentElement = modalElement.parentNode + + if (parentElement instanceof HTMLElement) { + console.debug('useLayoutEffect', { + modalElement, + parentElement, + }) + parentElement.scrollBy({ behavior: 'smooth', top: overflow, }) + } else { + modalElement.scrollBy({ + behavior: 'smooth', + top: overflow, + }) } } else { window.scrollBy({ behavior: 'smooth', top: overflow }) } } } - // oxlint-disable react/exhaustive-deps - }, [isDropdownVisible, refSelect, size, ref.current]) + }, [ + isDropdownVisible, + refSelect, + size, + modalContext, + theme.sizing, + theme.space, + ]) const resizeDropdown = useCallback(() => { if ( @@ -696,33 +751,24 @@ export const Dropdown = ({ setSearch('') } - if (!searchable) { - document.addEventListener('keydown', event => - handleKeyDown( - event, - ref, - options, - searchBarActive, - setSearch, - setDefaultSearch, - search, - ), + const eventKeydown = (event: globalThis.KeyboardEvent) => + handleKeyDown( + event, + ref, + options, + searchBarActive, + setSearch, + setDefaultSearch, + search, ) + + if (!searchable) { + document.addEventListener('keydown', eventKeydown) } return () => { if (!searchable) { - document.removeEventListener('keydown', event => - handleKeyDown( - event, - ref, - options, - searchBarActive, - setSearch, - setDefaultSearch, - search, - ), - ) + document.removeEventListener('keydown', eventKeydown) } } }, [ diff --git a/packages/ui/src/components/TagInput/index.tsx b/packages/ui/src/components/TagInput/index.tsx index 48e791c657..83d86a3e06 100644 --- a/packages/ui/src/components/TagInput/index.tsx +++ b/packages/ui/src/components/TagInput/index.tsx @@ -138,13 +138,13 @@ export const TagInput = ({ setTagInput(newTagInput) if (newTagInput.length !== tagInputState.length && newTagInput) { setStatus({ - [newTagInput[newTagInput.length - 1].index]: STATUS.LOADING, + [newTagInput.at(-1).index]: STATUS.LOADING, }) } try { dispatchOnChange(newTagInput) if (newTagInput) { - setStatus({ [newTagInput[newTagInput.length - 1].index]: STATUS.IDLE }) + setStatus({ [newTagInput.at(-1).index]: STATUS.IDLE }) } } catch { setTagInput(tagInputState) @@ -177,7 +177,7 @@ export const TagInput = ({ ) { event.preventDefault() if (tagInputState) { - deleteTag(tagInputState[tagInputState.length - 1].index) + deleteTag(tagInputState.at(-1).index) } } } diff --git a/packages/ui/src/components/TagList/index.tsx b/packages/ui/src/components/TagList/index.tsx index 6ced8a91ca..753dd8baf3 100644 --- a/packages/ui/src/components/TagList/index.tsx +++ b/packages/ui/src/components/TagList/index.tsx @@ -223,7 +223,7 @@ export const TagList = ({ const visibleTagsCopy = visibleTags.filter( (_, index) => index < visibleTags.length - 1, ) - const tagToMove = visibleTags[visibleTags.length - 1] ?? '' + const tagToMove = visibleTags.at(-1) ?? '' setVisibleTags(visibleTagsCopy) setHiddenTags([tagToMove, ...hiddenTags]) diff --git a/packages/ui/src/components/VerificationCode/index.tsx b/packages/ui/src/components/VerificationCode/index.tsx index a03920b2f8..bfa05cb806 100644 --- a/packages/ui/src/components/VerificationCode/index.tsx +++ b/packages/ui/src/components/VerificationCode/index.tsx @@ -133,7 +133,7 @@ export const VerificationCode = ({ const prevIndex = index - 1 const nextIndex = index + 1 const first = inputRefs[0] - const last = inputRefs[inputRefs.length - 1] + const last = inputRefs.at(-1) const prev = inputRefs[prevIndex] const next = inputRefs[nextIndex] const vals = [...values] diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7326252572..2a26c19e4d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -13301,8 +13301,8 @@ snapshots: '@typescript-eslint/parser': 8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3) eslint: 9.39.1(jiti@2.4.2) eslint-import-resolver-node: 0.3.9 - eslint-import-resolver-typescript: 3.8.7(eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)) - eslint-plugin-import: 2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-typescript@3.8.7(eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)) + eslint-import-resolver-typescript: 3.8.7(eslint-plugin-import@2.31.0)(eslint@9.39.1(jiti@2.4.2)) + eslint-plugin-import: 2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-typescript@3.8.7)(eslint@9.39.1(jiti@2.4.2)) eslint-plugin-jsx-a11y: 6.10.2(eslint@9.39.1(jiti@2.4.2)) eslint-plugin-react: 7.37.5(eslint@9.39.1(jiti@2.4.2)) eslint-plugin-react-hooks: 5.2.0(eslint@9.39.1(jiti@2.4.2)) @@ -13332,7 +13332,7 @@ snapshots: transitivePeerDependencies: - supports-color - eslint-import-resolver-typescript@3.8.7(eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)): + eslint-import-resolver-typescript@3.8.7(eslint-plugin-import@2.31.0)(eslint@9.39.1(jiti@2.4.2)): dependencies: '@nolyfill/is-core-module': 1.0.39 debug: 4.4.3 @@ -13343,7 +13343,7 @@ snapshots: stable-hash: 0.0.4 tinyglobby: 0.2.15 optionalDependencies: - eslint-plugin-import: 2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-typescript@3.8.7(eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)) + eslint-plugin-import: 2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-typescript@3.8.7)(eslint@9.39.1(jiti@2.4.2)) transitivePeerDependencies: - supports-color @@ -13373,14 +13373,14 @@ snapshots: transitivePeerDependencies: - supports-color - eslint-module-utils@2.12.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.8.7(eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)): + eslint-module-utils@2.12.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.8.7)(eslint@9.39.1(jiti@2.4.2)): dependencies: debug: 3.2.7 optionalDependencies: '@typescript-eslint/parser': 8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3) eslint: 9.39.1(jiti@2.4.2) eslint-import-resolver-node: 0.3.9 - eslint-import-resolver-typescript: 3.8.7(eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)) + eslint-import-resolver-typescript: 3.8.7(eslint-plugin-import@2.31.0)(eslint@9.39.1(jiti@2.4.2)) transitivePeerDependencies: - supports-color @@ -13419,7 +13419,7 @@ snapshots: - eslint-import-resolver-webpack - supports-color - eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-typescript@3.8.7(eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)): + eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-typescript@3.8.7)(eslint@9.39.1(jiti@2.4.2)): dependencies: '@rtsao/scc': 1.1.0 array-includes: 3.1.8 @@ -13430,7 +13430,7 @@ snapshots: doctrine: 2.1.0 eslint: 9.39.1(jiti@2.4.2) eslint-import-resolver-node: 0.3.9 - eslint-module-utils: 2.12.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.8.7(eslint-plugin-import@2.31.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)))(eslint@9.39.1(jiti@2.4.2)) + eslint-module-utils: 2.12.0(@typescript-eslint/parser@8.44.1(eslint@9.39.1(jiti@2.4.2))(typescript@5.9.3))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.8.7)(eslint@9.39.1(jiti@2.4.2)) hasown: 2.0.2 is-core-module: 2.16.1 is-glob: 4.0.3