-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Sticky offset in nested AppLayout #3013
Changes from 6 commits
6224d9b
c71ed10
0999947
475e7e3
1b24bf7
02e941e
eda341d
e1e7fc5
ed9e920
9a5290d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
import React from 'react'; | ||
|
||
import AppLayout from '~components/app-layout'; | ||
import Button from '~components/button'; | ||
import Header from '~components/header'; | ||
import Table from '~components/table'; | ||
|
||
import { generateItems, Instance } from '../table/generate-data'; | ||
import { columnsConfig } from '../table/shared-configs'; | ||
import ScreenshotArea from '../utils/screenshot-area'; | ||
import { Breadcrumbs, Navigation } from './utils/content-blocks'; | ||
import { Tools } from './utils/content-blocks'; | ||
import labels from './utils/labels'; | ||
import * as toolsContent from './utils/tools-content'; | ||
|
||
const items = generateItems(100); | ||
|
||
export default function () { | ||
return ( | ||
<ScreenshotArea gutters={false}> | ||
<AppLayout | ||
data-testid="main-layout" | ||
ariaLabels={labels} | ||
navigation={<Navigation />} | ||
toolsHide={true} | ||
disableContentPaddings={true} | ||
content={ | ||
<AppLayout | ||
data-testid="secondary-layout" | ||
ariaLabels={labels} | ||
breadcrumbs={<Breadcrumbs />} | ||
navigationHide={true} | ||
content={ | ||
<Table<Instance> | ||
header={ | ||
<Header | ||
variant="awsui-h1-sticky" | ||
description="Demo page with footer" | ||
actions={<Button variant="primary">Create</Button>} | ||
> | ||
Sticky Scrollbar Example | ||
</Header> | ||
} | ||
stickyHeader={true} | ||
variant="full-page" | ||
columnDefinitions={columnsConfig} | ||
items={items} | ||
/> | ||
} | ||
tools={<Tools>{toolsContent.long}</Tools>} | ||
/> | ||
} | ||
/> | ||
</ScreenshotArea> | ||
); | ||
} |
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, { useEffect, useImperativeHandle, useState } from 'react'; | ||||
import React, { useEffect, useImperativeHandle, useLayoutEffect, useRef, useState } from 'react'; | ||||
|
||||
import { useStableCallback } from '@cloudscape-design/component-toolkit/internal'; | ||||
|
||||
|
@@ -9,6 +9,7 @@ import { SplitPanelSideToggleProps } from '../../internal/context/split-panel-co | |||
import { fireNonCancelableEvent } from '../../internal/events'; | ||||
import { useControllable } from '../../internal/hooks/use-controllable'; | ||||
import { useIntersectionObserver } from '../../internal/hooks/use-intersection-observer'; | ||||
import { useMergeRefs } from '../../internal/hooks/use-merge-refs'; | ||||
import { useMobile } from '../../internal/hooks/use-mobile'; | ||||
import { useUniqueId } from '../../internal/hooks/use-unique-id'; | ||||
import { useGetGlobalBreadcrumbs } from '../../internal/plugins/helpers/use-global-breadcrumbs'; | ||||
|
@@ -78,6 +79,8 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa | |||
const [notificationsHeight, setNotificationsHeight] = useState(0); | ||||
const [navigationAnimationDisabled, setNavigationAnimationDisabled] = useState(true); | ||||
const [splitPanelAnimationDisabled, setSplitPanelAnimationDisabled] = useState(true); | ||||
const [isNested, setIsNested] = useState(false); | ||||
const rootRef = useRef<HTMLDivElement>(null); | ||||
|
||||
const [toolsOpen = false, setToolsOpen] = useControllable(controlledToolsOpen, onToolsChange, false, { | ||||
componentName: 'AppLayout', | ||||
|
@@ -432,16 +435,41 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa | |||
placement.inlineSize, | ||||
]); | ||||
|
||||
/** | ||||
* Returns true if the AppLayout is nested | ||||
* Does not apply to iframe | ||||
*/ | ||||
const getIsNestedInAppLayout = (element: HTMLElement | null): boolean => { | ||||
let currentElement: Element | null = element?.parentElement ?? null; | ||||
|
||||
while (currentElement) { | ||||
if (getComputedStyle(currentElement).getPropertyValue(globalVars.stickyVerticalTopOffset)) { | ||||
return true; | ||||
} | ||||
currentElement = currentElement.parentElement; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CSS var values propagate. You do not need to manually walk up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, the decision suggested by you is better, but it does not work in u tests env so I turned it back There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, so Let's leave a comment for the future readers this loop is needed for JSDOM. Fine to keep it because in the real browsers it will exit after the first iteration anyway |
||||
} | ||||
|
||||
return false; | ||||
}; | ||||
|
||||
useLayoutEffect(() => { | ||||
setIsNested(getIsNestedInAppLayout(rootRef.current)); | ||||
}, []); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @just-boris you previously suggested to skip this calculation if toolbar is empty, but it turns out this performance optimization might even backfire if we put it as a dependency here, since it will be recalculated every time this prop changes, so I decided to leave it as is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not understand this comment. Could you post here in the comments a possible code how you planned to handle the toolbar and which dependency concerned you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. useLayoutEffect(() => {
if (toolbarProps) {
setIsNested(getIsNestedInAppLayout(rootRef.current));
}
}, [toolbarProps]); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about using
|
||||
|
||||
return ( | ||||
<> | ||||
{/* Rendering a hidden copy of breadcrumbs to trigger their deduplication */} | ||||
{!hasToolbar && breadcrumbs ? <ScreenreaderOnly>{breadcrumbs}</ScreenreaderOnly> : null} | ||||
<SkeletonLayout | ||||
ref={intersectionObserverRef} | ||||
ref={useMergeRefs(intersectionObserverRef, rootRef)} | ||||
style={{ | ||||
[globalVars.stickyVerticalTopOffset]: `${verticalOffsets.header}px`, | ||||
[globalVars.stickyVerticalBottomOffset]: `${placement.insetBlockEnd}px`, | ||||
paddingBlockEnd: splitPanelOpen && splitPanelPosition === 'bottom' ? splitPanelReportedSize : '', | ||||
...(toolbarProps || !isNested | ||||
georgylobko marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
? { | ||||
[globalVars.stickyVerticalTopOffset]: `${verticalOffsets.header}px`, | ||||
[globalVars.stickyVerticalBottomOffset]: `${placement.insetBlockEnd}px`, | ||||
} | ||||
: {}), | ||||
...(!isMobile ? { minWidth: `${minContentWidth}px` } : {}), | ||||
}} | ||||
toolbar={ | ||||
|
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.
Need to ensure this dev page passes new integration tests added there: #3063
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.
It does