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

UI: Fix tags sort for browser back-compat #30547

Merged
merged 2 commits into from
Feb 14, 2025
Merged

Conversation

shilman
Copy link
Member

@shilman shilman commented Feb 14, 2025

Closes #30545

What I did

Downgrade from toSorted to sort for compatibility with older browsers.

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

Click the tags filter and observe that the tags are sorted.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

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

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 80.5 MB 80.6 MB 20.6 kB 3.21 0%
initSize 80.5 MB 80.6 MB 20.6 kB 3.21 0%
diffSize 97 B 97 B 0 B - 0%
buildSize 7.31 MB 7.31 MB 27 B 1.12 0%
buildSbAddonsSize 1.9 MB 1.9 MB 0 B 1.03 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.88 MB 1.88 MB 27 B 1.13 0%
buildSbPreviewSize 0 B 0 B 0 B - -
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.97 MB 3.97 MB 27 B 1.03 0%
buildPreviewSize 3.34 MB 3.34 MB 0 B 1.16 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 24.5s 22.8s -1s -699ms 0.44 -7.4%
generateTime 18.2s 26.6s 8.4s 5.94 🔺31.5%
initTime 4.2s 4.9s 621ms 1.07 12.7%
buildTime 9.2s 9.9s 659ms 1.32 🔺6.6%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 5.7s 4.8s -857ms -0.71 -17.6%
devManagerResponsive 4.2s 3.6s -620ms -0.74 -17%
devManagerHeaderVisible 812ms 736ms -76ms -0.42 -10.3%
devManagerIndexVisible 891ms 743ms -148ms -0.63 -19.9%
devStoryVisibleUncached 3.6s 3.2s -419ms -0.35 -13%
devStoryVisible 892ms 772ms -120ms -0.61 -15.5%
devAutodocsVisible 829ms 797ms -32ms 0.39 -4%
devMDXVisible 769ms 638ms -131ms -0.91 -20.5%
buildManagerHeaderVisible 650ms 815ms 165ms -0.29 20.2%
buildManagerIndexVisible 665ms 824ms 159ms -0.65 19.3%
buildStoryVisible 634ms 801ms 167ms -0.09 20.8%
buildAutodocsVisible 632ms 593ms -39ms -0.47 -6.6%
buildMDXVisible 626ms 537ms -89ms -1 -16.6%

Greptile Summary

This PR addresses a browser compatibility issue by replacing the .toSorted() array method with .sort() in the TagsFilter component to support older browsers like Chrome 91.

  • Changed array sorting method in /code/core/src/manager/components/sidebar/TagsFilter.tsx from .toSorted() to .sort()
  • Maintains same sorting functionality while improving browser compatibility for Chrome versions below 110
  • Simple but necessary fix to prevent Storybook crashes in older browsers
  • Change is covered by existing stories for manual testing verification

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +106 to +107
const tags = Array.from(allTags);
tags.sort();
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: mutates the original array since .sort() modifies in place. Consider using [...allTags] to create a new array

Suggested change
const tags = Array.from(allTags);
tags.sort();
const tags = [...Array.from(allTags)].sort();

@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Feb 14, 2025
@shilman shilman changed the title UI: Fix tags sort to be compatible with older browsers UI: Fix tags sort for browser compatibility Feb 14, 2025
@shilman shilman changed the title UI: Fix tags sort for browser compatibility UI: Fix tags sort for browser back-compat Feb 14, 2025
Copy link

nx-cloud bot commented Feb 14, 2025

View your CI Pipeline Execution ↗ for commit 0f256ad.

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 2m View ↗

☁️ Nx Cloud last updated this comment at 2025-02-14 11:54:28 UTC

@storybook-pr-benchmarking
Copy link

Package Benchmarks

Commit: 0f256ad, ran on 14 February 2025 at 11:59:27 UTC

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

@storybook/vue3-vite

Before After Difference
Dependency count 108 108 0
Self size 17 KB 17 KB 0 B
Dependency size 42.60 MB 42.61 MB 🚨 +12 KB 🚨
Bundle Size Analyzer Link Link

@storybook/vue3

Before After Difference
Dependency count 17 17 0
Self size 87 KB 87 KB 0 B
Dependency size 6.13 MB 6.14 MB 🚨 +12 KB 🚨
Bundle Size Analyzer Link Link

@shilman shilman merged commit be2ef66 into next Feb 14, 2025
62 of 65 checks passed
@shilman shilman deleted the shilman/30545-fix-tags-sort branch February 14, 2025 12:13
shilman added a commit that referenced this pull request Feb 14, 2025
UI: Fix tags sort for browser back-compat
(cherry picked from commit be2ef66)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Feb 14, 2025
@valentinpalkovic valentinpalkovic added needs qa Indicates that this needs manual QA during the upcoming minor/major release and removed needs qa Indicates that this needs manual QA during the upcoming minor/major release labels Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:normal compatibility with other tools patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Storybook crashes in Chrome 91 because of .toSorted method
2 participants