Skip to content
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

Core: Fix scrollIntoView behavior and reimplement testing module time rendering #30044

Merged
merged 6 commits into from
Dec 13, 2024
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
8 changes: 4 additions & 4 deletions code/addons/test/src/components/Description.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,10 @@ export function Description({ state, ...props }: DescriptionProps) {
);
} else if (state.progress?.finishedAt) {
description = (
<RelativeTime
timestamp={new Date(state.progress.finishedAt)}
testCount={state.progress.numTotalTests}
/>
<>
Ran {state.progress.numTotalTests} {state.progress.numTotalTests === 1 ? 'test' : 'tests'}{' '}
<RelativeTime timestamp={state.progress.finishedAt} />
</>
);
} else if (state.watching) {
description = 'Watching for file changes';
Expand Down
47 changes: 47 additions & 0 deletions code/addons/test/src/components/RelativeTime.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import type { Meta, StoryObj } from '@storybook/react';

import { RelativeTime } from './RelativeTime';

const meta = {
component: RelativeTime,
} satisfies Meta<typeof RelativeTime>;

export default meta;

type Story = StoryObj<typeof meta>;

export const JustNow: Story = {
args: {
timestamp: Date.now() - 1000 * 10,
},
};

export const AMinuteAgo: Story = {
args: {
timestamp: Date.now() - 1000 * 60,
},
};

export const MinutesAgo: Story = {
args: {
timestamp: Date.now() - 1000 * 60 * 2,
},
};

export const HoursAgo: Story = {
args: {
timestamp: Date.now() - 1000 * 60 * 60 * 3,
},
};

export const Yesterday: Story = {
args: {
timestamp: Date.now() - 1000 * 60 * 60 * 24,
},
};

export const DaysAgo: Story = {
args: {
timestamp: Date.now() - 1000 * 60 * 60 * 24 * 3,
},
};
56 changes: 25 additions & 31 deletions code/addons/test/src/components/RelativeTime.tsx
Original file line number Diff line number Diff line change
@@ -1,41 +1,35 @@
import { useEffect, useState } from 'react';

export function getRelativeTimeString(date: Date): string {
const delta = Math.round((date.getTime() - Date.now()) / 1000);
const cutoffs = [60, 3600, 86400, 86400 * 7, 86400 * 30, 86400 * 365, Infinity];
const units: Intl.RelativeTimeFormatUnit[] = [
'second',
'minute',
'hour',
'day',
'week',
'month',
'year',
];

const unitIndex = cutoffs.findIndex((cutoff) => cutoff > Math.abs(delta));
const divisor = unitIndex ? cutoffs[unitIndex - 1] : 1;
const rtf = new Intl.RelativeTimeFormat('en', { numeric: 'auto' });
return rtf.format(Math.floor(delta / divisor), units[unitIndex]);
}

export const RelativeTime = ({ timestamp, testCount }: { timestamp: Date; testCount: number }) => {
const [relativeTimeString, setRelativeTimeString] = useState(null);
export const RelativeTime = ({ timestamp }: { timestamp?: number }) => {
const [timeAgo, setTimeAgo] = useState(null);

useEffect(() => {
if (timestamp) {
setRelativeTimeString(getRelativeTimeString(timestamp).replace(/^now$/, 'just now'));

const interval = setInterval(() => {
setRelativeTimeString(getRelativeTimeString(timestamp).replace(/^now$/, 'just now'));
}, 10000);

setTimeAgo(Date.now() - timestamp);
const interval = setInterval(() => setTimeAgo(Date.now() - timestamp), 10000);
return () => clearInterval(interval);
}
}, [timestamp]);

return (
relativeTimeString &&
`Ran ${testCount} ${testCount === 1 ? 'test' : 'tests'} ${relativeTimeString}`
);
if (timeAgo === null) {
return null;
}

const seconds = Math.round(timeAgo / 1000);
if (seconds < 60) {
return `just now`;
}

const minutes = Math.floor(seconds / 60);
if (minutes < 60) {
return minutes === 1 ? `a minute ago` : `${minutes} minutes ago`;
}

const hours = Math.floor(minutes / 60);
if (hours < 24) {
return hours === 1 ? `an hour ago` : `${hours} hours ago`;
}

const days = Math.floor(hours / 24);
return days === 1 ? `yesterday` : `${days} days ago`;
};
31 changes: 23 additions & 8 deletions code/core/src/manager/components/sidebar/useHighlighted.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,25 @@ export interface HighlightedProps {
const fromSelection = (selection: Selection): Highlight =>
selection ? { itemId: selection.storyId, refId: selection.refId } : null;

const scrollToSelector = (
selector: string,
options: {
containerRef?: RefObject<Element | null>;
center?: boolean;
attempts?: number;
delay?: number;
} = {},
_attempt = 1
) => {
const { containerRef, center = false, attempts = 3, delay = 500 } = options;
const element = (containerRef ? containerRef.current : document)?.querySelector(selector);
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: optional chaining here could cause element to be undefined even when containerRef.current exists, if querySelector returns null

if (element) {
scrollIntoView(element, center);
} else if (_attempt <= attempts) {
setTimeout(scrollToSelector, delay, selector, options, _attempt + 1);
}
Comment on lines +39 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: recursive setTimeout calls could stack up if the selector matches multiple times during retries

};

export const useHighlighted = ({
containerRef,
isLoading,
Expand Down Expand Up @@ -65,14 +84,10 @@ export const useHighlighted = ({
const highlight = fromSelection(selected);
updateHighlighted(highlight);
if (highlight) {
const { itemId, refId } = highlight;
setTimeout(() => {
scrollIntoView(
// @ts-expect-error (non strict)
containerRef.current?.querySelector(`[data-item-id="${itemId}"][data-ref-id="${refId}"]`),
true // make sure it's clearly visible by centering it
);
}, 0);
scrollToSelector(`[data-item-id="${highlight.itemId}"][data-ref-id="${highlight.refId}"]`, {
containerRef,
center: true,
});
}
}, [containerRef, selected, updateHighlighted]);

Expand Down
12 changes: 8 additions & 4 deletions code/core/src/manager/utils/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,14 @@ export const scrollIntoView = (element: Element, center = false) => {
return;
}
const { top, bottom } = element.getBoundingClientRect();
const isInView =
top >= 0 && bottom <= (globalWindow.innerHeight || document.documentElement.clientHeight);

if (!isInView) {
if (!top || !bottom) {
return;
}
Comment on lines +88 to +90
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: this null check is redundant since getBoundingClientRect() always returns a DOMRect with numeric values

const bottomOffset =
document?.querySelector('#sidebar-bottom-wrapper')?.getBoundingClientRect().top ||
globalWindow.innerHeight ||
document.documentElement.clientHeight;
Comment on lines +91 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider caching bottomOffset value to avoid repeated DOM queries and reflows

if (bottom > bottomOffset) {
element.scrollIntoView({ block: center ? 'center' : 'nearest' });
}
Comment on lines +95 to 97
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: element may still be partially hidden if top < 0. Should also check if top is above viewport

};
Expand Down
Loading