Core: Fix `scrollIntoView` behavior and reimplement testing module time rendering by ghengeveld · Pull Request #30044 · storybookjs/storybook

ghengeveld · 2024-12-12T13:38:28Z

What I did

Updated the sidebar's scrollIntoView behavior to make the selected story visible when switching stories. It wasn't taking into account the testing module which may overlap the selected story, and wasn't triggering after creating a new story file through the UI.

Updated the RelativeTime logic to be more concise, less technical and more fitting to our use case. I didn't shorten "seconds" or "minutes" to "s" or "m" but instead made it less likely for those to show up.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-30044-sha-f88023ad. Try it out in a new sandbox by running npx storybook@0.0.0-pr-30044-sha-f88023ad sandbox or in an existing project with npx storybook@0.0.0-pr-30044-sha-f88023ad upgrade.

More information


Published version	`0.0.0-pr-30044-sha-f88023ad`
Triggered by	@ghengeveld
Repository	storybookjs/storybook
Branch	`ui-fixes`
Commit	`f88023ad`
Datetime	Fri Dec 13 09:05:01 UTC 2024 (`1734080701`)
Workflow run	12312680567

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=30044

name	before	after	diff	z	%
createSize	0 B	0 B	0 B	-	-
generateSize	77.7 MB	77.7 MB	0 B	1.22	0%
initSize	133 MB	133 MB	-10.5 kB	0.91	0%
diffSize	55.3 MB	55.3 MB	-10.5 kB	0.9	0%
buildSize	6.87 MB	6.87 MB	1.03 kB	1.49	0%
buildSbAddonsSize	1.51 MB	1.51 MB	843 B	1.11	0.1%
buildSbCommonSize	195 kB	195 kB	0 B	-	0%
buildSbManagerSize	1.86 MB	1.86 MB	175 B	28.17	0%
buildSbPreviewSize	0 B	0 B	0 B	-	-
buildStaticSize	0 B	0 B	0 B	-	-
buildPrebuildSize	3.57 MB	3.57 MB	1.02 kB	1.49	0%
buildPreviewSize	3.3 MB	3.3 MB	8 B	1	0%
testBuildSize	0 B	0 B	0 B	-	-
testBuildSbAddonsSize	0 B	0 B	0 B	-	-
testBuildSbCommonSize	0 B	0 B	0 B	-	-
testBuildSbManagerSize	0 B	0 B	0 B	-	-
testBuildSbPreviewSize	0 B	0 B	0 B	-	-
testBuildStaticSize	0 B	0 B	0 B	-	-
testBuildPrebuildSize	0 B	0 B	0 B	-	-
testBuildPreviewSize	0 B	0 B	0 B	-	-

name	before	after	diff	z	%
createTime	23.9s	23.3s	-590ms	1.3	-2.5%
generateTime	18.8s	19.3s	526ms	-0.69	2.7%
initTime	12.3s	12.9s	537ms	-0.76	4.2%
buildTime	8.8s	9.3s	447ms	0	4.8%
testBuildTime	0ms	0ms	0ms	-	-
devPreviewResponsive	5.9s	6.3s	412ms	1.71	🔺6.5%
devManagerResponsive	4.2s	4.6s	438ms	1.89	🔺9.3%
devManagerHeaderVisible	582ms	723ms	141ms	1.58	🔺19.5%
devManagerIndexVisible	657ms	768ms	111ms	1.51	🔺14.5%
devStoryVisibleUncached	2s	1.9s	-32ms	0.5	-1.6%
devStoryVisible	655ms	824ms	169ms	2.11	🔺20.5%
devAutodocsVisible	593ms	555ms	-38ms	0.33	-6.8%
devMDXVisible	665ms	469ms	-196ms	-0.64	-41.8%
buildManagerHeaderVisible	620ms	742ms	122ms	1.08	16.4%
buildManagerIndexVisible	734ms	767ms	33ms	0.49	4.3%
buildStoryVisible	556ms	674ms	118ms	1.09	17.5%
buildAutodocsVisible	442ms	595ms	153ms	1.14	25.7%
buildMDXVisible	458ms	500ms	42ms	0.45	8.4%

Greptile Summary

Improved sidebar scroll behavior and simplified the RelativeTime component's implementation for the testing module, focusing on better visibility and user-friendly time displays.

Modified code/core/src/manager/utils/tree.ts to check for testing module overlap when scrolling stories into view
Added retry mechanism in code/core/src/manager/components/sidebar/useHighlighted.ts for more reliable story scrolling
Simplified code/addons/test/src/components/RelativeTime.tsx to use direct time calculations instead of Intl.RelativeTimeFormat
Added comprehensive story coverage in RelativeTime.stories.tsx for various time intervals
Improved error handling with null checks for getBoundingClientRect() values

… to scroll the selected story into view

greptile-apps

_{4 file(s) reviewed, 9 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-12-12T13:38:57Z

code/addons/test/src/components/RelativeTime.stories.tsx

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


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

greptile-apps · 2024-12-12T13:38:57Z

code/addons/test/src/components/RelativeTime.stories.tsx

+const meta = {
+  component: RelativeTime,
+  args: {
+    testCount: 42,


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

greptile-apps · 2024-12-12T13:38:59Z

code/addons/test/src/components/RelativeTime.tsx

-    'month',
-    'year',
-  ];
+  const seconds = Math.round((Date.now() - date.getTime()) / 1000);


logic: This calculation assumes the timestamp is in the past - future dates will show incorrect values

greptile-apps · 2024-12-12T13:39:00Z

code/addons/test/src/components/RelativeTime.tsx

 }

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


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

greptile-apps · 2024-12-12T13:39:02Z

code/core/src/manager/utils/tree.ts

+  if (!top || !bottom) {
+    return;
+  }


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

greptile-apps · 2024-12-12T13:39:02Z

code/core/src/manager/components/sidebar/useHighlighted.ts

+  const { containerRef, center = false, attempts = 3, delay = 500 } = options;
+  const element = (containerRef ? containerRef.current : document)?.querySelector(selector);


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

greptile-apps · 2024-12-12T13:39:02Z

code/core/src/manager/utils/tree.ts

+  const bottomOffset =
+    document?.querySelector('#sidebar-bottom-wrapper')?.getBoundingClientRect().top ||
+    globalWindow.innerHeight ||
+    document.documentElement.clientHeight;


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

greptile-apps · 2024-12-12T13:39:03Z

code/core/src/manager/components/sidebar/useHighlighted.ts

+  } else if (_attempt <= attempts) {
+    setTimeout(scrollToSelector, delay, selector, options, _attempt + 1);
+  }


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

greptile-apps · 2024-12-12T13:39:03Z

code/core/src/manager/utils/tree.ts

+  if (bottom > bottomOffset) {
    element.scrollIntoView({ block: center ? 'center' : 'nearest' });
  }


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

nx-cloud · 2024-12-12T13:43:13Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f88023a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run-many -t build --parallel=3

Sent with 💌 from NxCloud.

storybook-app-bot · 2024-12-12T13:52:18Z

Package Benchmarks

^{Commit: f88023a, ran on 13 December 2024 at 08:45:30 UTC}

The following packages have significant changes to their size or dependencies:

`@storybook/addon-a11y`

	Before	After	Difference
Dependency count	59	59	0
Self size	411 KB	45 KB	🎉 -366 KB 🎉
Dependency size	13.46 MB	13.46 MB	0 B
Bundle Size Analyzer	Link	Link

`@storybook/addon-onboarding`

	Before	After	Difference
Dependency count	0	2	🚨 +2 🚨
Self size	235 KB	216 KB	🎉 -19 KB 🎉
Dependency size	670 B	235 KB	🚨 +235 KB 🚨
Bundle Size Analyzer	Link	Link

`@storybook/experimental-addon-test`

	Before	After	Difference
Dependency count	60	60	0
Self size	972 KB	958 KB	🎉 -15 KB 🎉
Dependency size	14.14 MB	14.14 MB	0 B
Bundle Size Analyzer	Link	Link

`@storybook/core`

	Before	After	Difference
Dependency count	49	50	🚨 +1 🚨
Self size	21.44 MB	21.44 MB	🚨 +237 B 🚨
Dependency size	14.34 MB	14.36 MB	🚨 +16 KB 🚨
Bundle Size Analyzer	Link	Link

`@storybook/experimental-nextjs-vite`

	Before	After	Difference
Dependency count	153	87	🎉 -66 🎉
Self size	231 KB	231 KB	🎉 -152 B 🎉
Dependency size	44.67 MB	31.57 MB	🎉 -13.10 MB 🎉
Bundle Size Analyzer	Link	Link

`storybook`

	Before	After	Difference
Dependency count	50	51	🚨 +1 🚨
Self size	22 KB	22 KB	0 B
Dependency size	35.78 MB	35.79 MB	🚨 +16 KB 🚨
Bundle Size Analyzer	Link	Link

`sb`

	Before	After	Difference
Dependency count	51	52	🚨 +1 🚨
Self size	1 KB	1 KB	0 B
Dependency size	35.80 MB	35.82 MB	🚨 +16 KB 🚨
Bundle Size Analyzer	Link	Link

`@storybook/cli`

	Before	After	Difference
Dependency count	392	393	🚨 +1 🚨
Self size	493 KB	493 KB	0 B
Dependency size	77.38 MB	77.40 MB	🚨 +16 KB 🚨
Bundle Size Analyzer	Link	Link

`@storybook/codemod`

	Before	After	Difference
Dependency count	272	273	🚨 +1 🚨
Self size	612 KB	612 KB	0 B
Dependency size	67.38 MB	67.39 MB	🚨 +16 KB 🚨
Bundle Size Analyzer	Link	Link

`create-storybook`

	Before	After	Difference
Dependency count	108	109	🚨 +1 🚨
Self size	1.11 MB	1.11 MB	0 B
Dependency size	44.91 MB	44.93 MB	🚨 +16 KB 🚨
Bundle Size Analyzer	Link	Link

ghengeveld added 4 commits December 12, 2024 13:33

Retry scrolling to newly created story

afcb243

Consider height of the testing module and notifications when deciding…

d5bdc51

… to scroll the selected story into view

Reimplement time rendering logic and add stories

6c9d375

Refactor for clarity

508c825

ghengeveld added bug ci:normal labels Dec 12, 2024

ghengeveld requested review from JReinhold and ndelangen December 12, 2024 13:38

greptile-apps bot reviewed Dec 12, 2024

View reviewed changes

ghengeveld added 2 commits December 12, 2024 15:05

Pull test count out of RelativeTime

20dfae6

Remove stray dollar sign

f88023a

JReinhold approved these changes Dec 13, 2024

View reviewed changes

ghengeveld merged commit 6e0da96 into next Dec 13, 2024

ghengeveld deleted the ui-fixes branch December 13, 2024 10:02

github-actions bot mentioned this pull request Dec 13, 2024

Release: Prerelease 8.5.0-alpha.22 #30041

Merged

17 tasks

yannbf assigned ghengeveld Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

Core: Fix `scrollIntoView` behavior and reimplement testing module time rendering#30044
ghengeveld merged 6 commits intonextfrom
ui-fixes

ghengeveld commented Dec 12, 2024 •

edited by storybook-bot

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

greptile-apps bot Dec 12, 2024

Uh oh!

nx-cloud bot commented Dec 12, 2024 •

edited

Loading

Uh oh!

storybook-app-bot bot commented Dec 12, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const { containerRef, center = false, attempts = 3, delay = 500 } = options;
		const element = (containerRef ? containerRef.current : document)?.querySelector(selector);

Uh oh!

Conversation

ghengeveld commented Dec 12, 2024 • edited by storybook-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

Greptile Summary

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

nx-cloud bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

storybook-app-bot bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Package Benchmarks

@storybook/addon-a11y

@storybook/addon-onboarding

@storybook/experimental-addon-test

@storybook/core

@storybook/experimental-nextjs-vite

storybook

sb

@storybook/cli

@storybook/codemod

create-storybook

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghengeveld commented Dec 12, 2024 •

edited by storybook-bot

Loading

nx-cloud bot commented Dec 12, 2024 •

edited

Loading

storybook-app-bot bot commented Dec 12, 2024 •

edited

Loading

`@storybook/addon-a11y`

`@storybook/addon-onboarding`

`@storybook/experimental-addon-test`

`@storybook/core`

`@storybook/experimental-nextjs-vite`

`storybook`

`sb`

`@storybook/cli`

`@storybook/codemod`

`create-storybook`