fix: AppLayout horizontal scroll #2839
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import React, { useCallback, useEffect, useImperativeHandle, useState } from 'react'; | ||
|
|
||
| import ScreenreaderOnly from '../../internal/components/screenreader-only'; | ||
| import { SplitPanelSideToggleProps } from '../../internal/context/split-panel-context'; | ||
|
|
@@ -74,11 +74,6 @@ | |
| const [toolbarHeight, setToolbarHeight] = useState(0); | ||
| const [notificationsHeight, setNotificationsHeight] = useState(0); | ||
|
|
||
| const [toolsOpen = false, setToolsOpen] = useControllable(controlledToolsOpen, onToolsChange, false, { | ||
| componentName: 'AppLayout', | ||
| controlledProp: 'toolsOpen', | ||
|
|
@@ -116,27 +111,15 @@ | |
| // and compare a given number with the new drawer id min size | ||
|
|
||
| // the total size of all global drawers resized to their min size | ||
|
There was a problem hiding this comment. I realized that this calculation works incorrectly, as it returns min size only if |
||
| const totalActiveDrawersMinSize = getActiveDrawersTotalMinSize(); | ||
|
|
||
| const availableSpaceForNewDrawer = resizableSpaceAvailable - totalActiveDrawersMinSize; | ||
| if (availableSpaceForNewDrawer >= newDrawerSize) { | ||
| return; | ||
| } | ||
|
|
||
| // now we made sure we cannot accommodate the new drawer with existing ones | ||
| closeFirstDrawer(); | ||
| }; | ||
|
|
||
| const { | ||
|
|
@@ -164,6 +147,23 @@ | |
| onToolsToggle, | ||
| }); | ||
|
|
||
| const getActiveDrawersTotalMinSize = useCallback(() => { | ||
| const combinedDrawers = [...(drawers || []), ...globalDrawers]; | ||
| let totalActiveDrawersMinSize = activeGlobalDrawersIds | ||
| .map(activeDrawerId => | ||
| Math.min( | ||
| combinedDrawers.find(drawer => drawer.id === activeDrawerId)?.defaultSize ?? MIN_DRAWER_SIZE, | ||
| MIN_DRAWER_SIZE | ||
| ) | ||
| ) | ||
| .reduce((acc, curr) => acc + curr, 0); | ||
| if (activeDrawer) { | ||
| totalActiveDrawersMinSize += Math.min(activeDrawer?.defaultSize ?? MIN_DRAWER_SIZE, MIN_DRAWER_SIZE); | ||
| } | ||
|
|
||
| return totalActiveDrawersMinSize; | ||
| }, [activeDrawer, activeGlobalDrawersIds, drawers, globalDrawers]); | ||
|
|
||
| const onActiveDrawerChangeHandler = (drawerId: string | null) => { | ||
| onActiveDrawerChange(drawerId); | ||
| drawersFocusControl.setFocus(); | ||
|
|
@@ -227,6 +227,14 @@ | |
| const navigationFocusControl = useFocusControl(navigationOpen); | ||
| const splitPanelFocusControl = useSplitPanelFocusControl([splitPanelPreferences, splitPanelOpen]); | ||
|
|
||
| const onNavigationToggle = useCallback( | ||
|
There was a problem hiding this comment. Moved this handler below because it relies on navigationFocusControl, which is declared earlier There was a problem hiding this comment. We should use our There is no promise that To an extent, the same applies to other |
||
| (open: boolean) => { | ||
| navigationFocusControl.setFocus(); | ||
| fireNonCancelableEvent(onNavigationChange, { open }); | ||
| }, | ||
| [navigationFocusControl, onNavigationChange] | ||
| ); | ||
|
|
||
| useImperativeHandle(forwardRef, () => ({ | ||
| closeNavigationIfNecessary: () => isMobile && onNavigationToggle(false), | ||
| openTools: () => onToolsToggle(true), | ||
|
|
@@ -361,6 +369,15 @@ | |
| refs: splitPanelFocusControl.refs, | ||
| }; | ||
|
|
||
| const closeFirstDrawer = useCallback(() => { | ||
| const drawerToClose = drawersOpenQueue[drawersOpenQueue.length - 1]; | ||
| if (activeDrawer && activeDrawer?.id === drawerToClose) { | ||
| onActiveDrawerChange(null); | ||
| } else if (activeGlobalDrawersIds.includes(drawerToClose)) { | ||
| onActiveGlobalDrawersChange(drawerToClose); | ||
| } | ||
| }, [activeDrawer, activeGlobalDrawersIds, drawersOpenQueue, onActiveDrawerChange, onActiveGlobalDrawersChange]); | ||
|
|
||
| useEffect(() => { | ||
| // Close navigation drawer on mobile so that the main content is visible | ||
| if (isMobile) { | ||
|
|
@@ -369,6 +386,36 @@ | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isMobile]); | ||
|
|
||
| useEffect(() => { | ||
| if (isMobile) { | ||
| return; | ||
| } | ||
|
|
||
| const totalActiveDrawersMinSize = getActiveDrawersTotalMinSize(); | ||
| const activeNavigationWidth = navigationOpen ? navigationWidth : 0; | ||
| // collapsed content width is $space-layout-content-horizontal * 2 = 48px | ||
| const minContentVisibleWidth = 48; | ||
|
There was a problem hiding this comment. There’s no way to calculate this directly from the DOM tree since If you have a better idea, feel free to share it in this thread. There was a problem hiding this comment. We already have this constant: I suggest to reuse it somehow |
||
|
|
||
| const scrollWidth = activeNavigationWidth + minContentVisibleWidth + totalActiveDrawersMinSize; | ||
| const hasHorizontalScroll = scrollWidth > placement.inlineSize; | ||
| if (hasHorizontalScroll) { | ||
| if (navigationOpen) { | ||
| onNavigationToggle(false); | ||
| return; | ||
| } | ||
|
|
||
| closeFirstDrawer(); | ||
|
There was a problem hiding this comment. Didn't we want to have an iteration here, "close until hasHorizontalScroll is false"? There was a problem hiding this comment. No, we don't, because closing a drawer triggers a change in the dependency array, which leads to recalculations until the horizontal scrollbar is no longer present There was a problem hiding this comment. Not instantly obvious, but okay |
||
| } | ||
| }, [ | ||
| closeFirstDrawer, | ||
| getActiveDrawersTotalMinSize, | ||
| isMobile, | ||
| navigationOpen, | ||
| navigationWidth, | ||
| onNavigationToggle, | ||
| placement.inlineSize, | ||
| ]); | ||
|
|
||
| return ( | ||
| <> | ||
| {/* Rendering a hidden copy of breadcrumbs to trigger their deduplication */} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that this test suite didn't run earlier due to an extra wrap with an arrow function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks