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 4 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
50 changes: 50 additions & 0 deletions code/addons/test/src/components/RelativeTime.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import type { Meta, StoryObj } from '@storybook/react';

import { RelativeTime } from './RelativeTime';

const meta = {
component: RelativeTime,
args: {
testCount: 42,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: testCount of 42 seems arbitrary - consider using a more meaningful default that demonstrates singular/plural handling

},
} satisfies Meta<typeof RelativeTime>;

export default meta;

type Story = StoryObj<typeof meta>;

export const JustNow: Story = {
args: {
timestamp: new Date(),
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: using new Date() will cause this story to show different results each time it's rendered, making visual regression testing unreliable


export const SecondsAgo: Story = {
args: {
timestamp: new Date(Date.now() - 1000 * 15),
},
};

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

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

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

export const DaysAgo: Story = {
args: {
timestamp: new Date(Date.now() - 1000 * 60 * 60 * 24 * 3),
},
};
51 changes: 25 additions & 26 deletions code/addons/test/src/components/RelativeTime.tsx
Original file line number Diff line number Diff line change
@@ -1,38 +1,37 @@
import { useEffect, useState } from 'react';
import { useCallback, 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 seconds = Math.round((Date.now() - date.getTime()) / 1000);
if (seconds < 60) {
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 calculation assumes the timestamp is in the past - future dates will show incorrect values

return 'just now';
}

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]);
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`;
}

export const RelativeTime = ({ timestamp, testCount }: { timestamp: Date; testCount: number }) => {
const [relativeTimeString, setRelativeTimeString] = useState(null);
const updateRelativeTimeString = useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

style: useState should specify type: useState<string | null>(null)

() => timestamp && setRelativeTimeString(getRelativeTimeString(timestamp)),
[timestamp]
);

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

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

return () => clearInterval(interval);
}
}, [timestamp]);
updateRelativeTimeString();
const interval = setInterval(updateRelativeTimeString, 10000);
return () => clearInterval(interval);
}, [updateRelativeTimeString]);

return (
relativeTimeString &&
Expand Down
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