Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 24 additions & 23 deletions src/flashbar/collapsible-flashbar.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
import React, { ReactNode, useCallback, useLayoutEffect, useMemo, useRef, useState } from 'react';

import React, { ReactNode, useCallback, useLayoutEffect, useRef, useState } from 'react';
import { TransitionGroup } from 'react-transition-group';
import clsx from 'clsx';

Expand All @@ -17,8 +18,8 @@ import { getVisualContextClassname } from '../internal/components/visual-context
import customCssProps from '../internal/generated/custom-css-properties';
import { useDebounceCallback } from '../internal/hooks/use-debounce-callback';
import { useEffectOnUpdate } from '../internal/hooks/use-effect-on-update';
import { useThrottleCallback } from '../internal/hooks/use-throttle-callback';
import { scrollElementIntoView } from '../internal/utils/scrollable-containers';
import { throttle } from '../internal/utils/throttle';
import {
GeneratedAnalyticsMetadataFlashbarCollapse,
GeneratedAnalyticsMetadataFlashbarExpand,
Expand Down Expand Up @@ -121,28 +122,28 @@ export default function CollapsibleFlashbar({ items, style, ...restProps }: Inte
}
}, [isFlashbarStackExpanded]);

const updateBottomSpacing = useMemo(
() =>
throttle(() => {
// Allow vertical space between Flashbar and page bottom only when the Flashbar is reaching the end of the page,
// otherwise avoid spacing with eventual sticky elements below.
const listElement = listElementRef?.current;
const flashbar = listElement?.parentElement;
if (listElement && flashbar) {
// Make sure the bottom padding is present when we make the calculations,
// then we might decide to remove it or not.
flashbar.classList.remove(styles.floating);
const windowHeight = window.innerHeight;
// Take the parent region into account if using the App Layout, because it might have additional margins.
// Otherwise we use the Flashbar component for this calculation.
const outerElement = findUpUntil(flashbar, element => element.getAttribute('role') === 'region') || flashbar;
const applySpacing =
isFlashbarStackExpanded && Math.ceil(outerElement.getBoundingClientRect().bottom) >= windowHeight;
if (!applySpacing) {
flashbar.classList.add(styles.floating);
}
const updateBottomSpacing = useThrottleCallback(
() => {
// Allow vertical space between Flashbar and page bottom only when the Flashbar is reaching the end of the page,
// otherwise avoid spacing with eventual sticky elements below.
const listElement = listElementRef?.current;
const flashbar = listElement?.parentElement;
if (listElement && flashbar) {
// Make sure the bottom padding is present when we make the calculations,
// then we might decide to remove it or not.
flashbar.classList.remove(styles.floating);
const windowHeight = window.innerHeight;
// Take the parent region into account if using the App Layout, because it might have additional margins.
// Otherwise we use the Flashbar component for this calculation.
const outerElement = findUpUntil(flashbar, element => element.getAttribute('role') === 'region') || flashbar;
const applySpacing =
isFlashbarStackExpanded && Math.ceil(outerElement.getBoundingClientRect().bottom) >= windowHeight;
if (!applySpacing) {
flashbar.classList.add(styles.floating);
}
}, resizeListenerThrottleDelay),
}
},
resizeListenerThrottleDelay,
[isFlashbarStackExpanded]
);

Expand Down
15 changes: 15 additions & 0 deletions src/internal/hooks/use-throttle-callback/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

import { useMemo } from 'react';

import { throttle, ThrottledFunction } from '../../utils/throttle';

export function useThrottleCallback<F extends (...args: any) => any>(
func: F,
delay: number,
deps: React.DependencyList
): ThrottledFunction<F> {
// eslint-disable-next-line react-hooks/exhaustive-deps
return useMemo(() => throttle(func, delay), deps);
}
14 changes: 8 additions & 6 deletions src/progress-bar/index.tsx
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
'use client';
import React, { useEffect, useMemo, useState } from 'react';
import React, { useEffect, useState } from 'react';
import clsx from 'clsx';

import { useUniqueId, warnOnce } from '@cloudscape-design/component-toolkit/internal';

import { getBaseProps } from '../internal/base-component';
import { fireNonCancelableEvent } from '../internal/events';
import useBaseComponent from '../internal/hooks/use-base-component';
import { useThrottleCallback } from '../internal/hooks/use-throttle-callback';
import { applyDisplayName } from '../internal/utils/apply-display-name';
import { joinStrings } from '../internal/utils/strings';
import { throttle } from '../internal/utils/throttle';
import InternalLiveRegion from '../live-region/internal';
import { ProgressBarProps } from './interfaces';
import { Progress, ResultState, SmallText } from './internal';
Expand Down Expand Up @@ -52,11 +52,13 @@ export default function ProgressBar({
const additionalInfoId = useUniqueId('progressbar-additional-info-');

const [announcedValue, setAnnouncedValue] = useState('');
const throttledAssertion = useMemo(() => {
return throttle((value: ProgressBarProps['value']) => {
const throttledAssertion = useThrottleCallback(
(value: ProgressBarProps['value']) => {
setAnnouncedValue(`${value}%`);
}, ASSERTION_FREQUENCY);
}, []);
},
ASSERTION_FREQUENCY,
[]
);

useEffect(() => {
throttledAssertion(value);
Expand Down
13 changes: 12 additions & 1 deletion src/table/__tests__/resizable-columns.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ function findDragHandle() {
return new InternalDragHandleWrapper(document.body);
}

async function waitForResizeThrottle() {
await new Promise(resolve => setTimeout(resolve, 50));
}

afterEach(() => {
jest.restoreAllMocks();
});
Expand Down Expand Up @@ -235,18 +239,25 @@ test('takes width as min width if it is less than 120px and min width is not set
expect(wrapper.findColumnHeaders()[0].getElement()).toHaveStyle({ width: '100px' });
});

test('should follow along each mouse move event', () => {
test('should follow along each mouse move event', async () => {
const { wrapper } = renderTable(<Table {...defaultProps} />);
expect(wrapper.findColumnHeaders()[0].getElement()).toHaveStyle({ width: '150px' });

firePointerdown(wrapper.findColumnResizer(1)!);
firePointermove(200);
await waitForResizeThrottle();
expect(wrapper.findColumnHeaders()[0].getElement()).toHaveStyle({ width: '200px' });

firePointermove(250);
await waitForResizeThrottle();
expect(wrapper.findColumnHeaders()[0].getElement()).toHaveStyle({ width: '250px' });

firePointermove(200);
await waitForResizeThrottle();
expect(wrapper.findColumnHeaders()[0].getElement()).toHaveStyle({ width: '200px' });

firePointerup(200);
await waitForResizeThrottle();
expect(wrapper.findColumnHeaders()[0].getElement()).toHaveStyle({ width: '200px' });
});

Expand Down
6 changes: 5 additions & 1 deletion src/table/resizer/index.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

import React, { useCallback, useEffect, useRef, useState } from 'react';
import clsx from 'clsx';

Expand All @@ -8,6 +9,7 @@ import { getIsRtl, getLogicalBoundingClientRect, getLogicalPageX } from '@clouds
import { useSingleTabStopNavigation } from '@cloudscape-design/component-toolkit/internal';

import DragHandleWrapper from '../../internal/components/drag-handle-wrapper';
import { useThrottleCallback } from '../../internal/hooks/use-throttle-callback';
import { useVisualRefresh } from '../../internal/hooks/use-visual-mode';
import { KeyCode } from '../../internal/keycode';
import handleKey, { isEventLike } from '../../internal/utils/handle-key';
Expand All @@ -30,6 +32,7 @@ interface ResizerProps {
isBorderless: boolean;
}

const RESIZE_THROTTLE = 25;
const AUTO_GROW_START_TIME = 10;
const AUTO_GROW_INTERVAL = 10;
const AUTO_GROW_INCREMENT = 5;
Expand Down Expand Up @@ -168,7 +171,7 @@ export function Resizer({
[minWidth, onWidthUpdate, updateTrackerPosition]
);

const resizeColumn = useCallback(
const resizeColumn = useThrottleCallback(
(offset: number) => {
const elements = getResizerElements(resizerToggleRef.current);
if (!elements) {
Expand All @@ -183,6 +186,7 @@ export function Resizer({
updateColumnWidth(newWidth);
}
},
RESIZE_THROTTLE,
[updateColumnWidth]
);

Expand Down
5 changes: 0 additions & 5 deletions src/table/use-sticky-header.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@ export const useStickyHeader = (
secondaryTableRef.current &&
tableWrapperRef.current
) {
// Using the tableRef getBoundingClientRect().width instead of the theadRef because in VR
// the tableRef adds extra padding to the table and by default the theadRef will have a width
// without the padding and will make the sticky header width incorrect.
secondaryTableRef.current.style.inlineSize = `${tableRef.current.getBoundingClientRect().width}px`;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table column width utils set widths of both normal and sticky headers at the same time, so there is no need to sync table widths.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table column width utils set widths of both normal and sticky headers at the same time

Could you point out to the code that does that and why did you change it in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly! Here is where the col widths are synchronised: https://github.com/cloudscape-design/components/blob/main/src/table/use-column-widths.tsx#L141

When researching columns trembling, I realised that the large part of it was due to this line of code (only when sticky header is on). Here we measure the total size of the real table and set it to the sticky table. Due to the async nature of the code, this measurement is not that precise - and often happens before the column widths are settled - and thus contributing to the trembling. I tried to optimise it first, but then realise that this piece of code seems to be no longer relevant, as the same explicit sizes are used for all columns anyways.


tableWrapperRef.current.style.marginBlockStart = `-${theadRef.current.getBoundingClientRect().height}px`;
}
}, [theadRef, secondaryTheadRef, secondaryTableRef, tableWrapperRef, tableRef]);
Expand Down
Loading