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

Blocks: Fix scroll to non-ascii anchors #28826

Merged
merged 4 commits into from
Aug 19, 2024

Conversation

SkReD
Copy link
Contributor

@SkReD SkReD commented Aug 7, 2024

Closes #

What I did

Anchor scroll in docs not working for cyrillic symbols

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

  1. Visit the doc: https://635781f3500dd2c49e189caf-exszjlehcs.chromatic.com/?path=/docs/addons-docs-utfsymbolsscroll--docs
  2. Click the link
  3. Ensure it scrolls
  4. Visit the doc https://635781f3500dd2c49e189caf-exszjlehcs.chromatic.com/?path=/docs/addons-docs-toc-basic--docs
  5. Make the browser window so short that it can only show a single story
  6. Click "Two" in the right-hand-side TOC.
  7. Ensure it scrolls.

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/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 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 76.3 MB 76.3 MB 0 B 0.41 0%
initSize 169 MB 167 MB -1.51 MB -0.96 -0.9%
diffSize 92.7 MB 91.1 MB -1.51 MB -0.97 -1.7%
buildSize 7.46 MB 7.42 MB -40.5 kB -1 -0.5%
buildSbAddonsSize 1.61 MB 1.61 MB -9 B -0.71 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.3 MB 2.29 MB -13.2 kB -1 -0.6%
buildSbPreviewSize 351 kB 351 kB 243 B 0.89 0.1%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.46 MB 4.45 MB -13 kB -1 -0.3%
buildPreviewSize 3 MB 2.97 MB -27.4 kB -1 -0.9%
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 6.9s 8.5s 1.5s -0.9 18.2%
generateTime 19.7s 21.8s 2.1s 0.83 9.7%
initTime 16.3s 18.1s 1.7s 0.05 9.8%
buildTime 11.5s 10.5s -954ms -1.43 🔰-9%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 6.7s 6.1s -579ms -1.44 🔰-9.4%
devManagerResponsive 4.4s 4.1s -385ms -1.47 🔰-9.4%
devManagerHeaderVisible 737ms 695ms -42ms -1.52 🔰-6%
devManagerIndexVisible 780ms 754ms -26ms -1.2 -3.4%
devStoryVisibleUncached 1.1s 1.1s -29ms -1.07 -2.6%
devStoryVisible 780ms 753ms -27ms -1.26 -3.6%
devAutodocsVisible 606ms 660ms 54ms -0.78 8.2%
devMDXVisible 628ms 663ms 35ms -0.65 5.3%
buildManagerHeaderVisible 596ms 691ms 95ms -0.48 13.7%
buildManagerIndexVisible 603ms 693ms 90ms -0.52 13%
buildStoryVisible 634ms 765ms 131ms -0.16 17.1%
buildAutodocsVisible 600ms 615ms 15ms -1.03 2.4%
buildMDXVisible 543ms 610ms 67ms -0.68 11%

Greptile Summary

The PR modifies DocsContainer.tsx to fix anchor scrolling for non-ASCII characters by decoding the URL hash before locating the element.

  • Updated /code/lib/blocks/src/blocks/DocsContainer.tsx to decode URL hash for non-ASCII anchor support.
  • Ensures elements with non-ASCII IDs are correctly scrolled to by decoding url.hash.
  • Introduced a delay in useEffect to ensure scrolling works on full refresh.
  • No significant pitfalls or security issues expected, but verify documentation navigation remains unaffected.

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.

LGTM

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

Copy link

nx-cloud bot commented Aug 12, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d20b3c6. 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

Sent with 💌 from NxCloud.

@ndelangen
Copy link
Member

@SkReD Thank you for this PR!

Its a small change, I agree however, I think a manual steps to reproduce should be defined on this PR, as there is also no issue linked.
The PR should provide more context, for future us.

Even better would be a story somewhere where we can quickly verify the correct behavior.

@ndelangen ndelangen changed the title Fix docs scroll for non-ascii anchors Addon-Docs: Fix scroll to non-ascii anchors Aug 14, 2024
@ndelangen ndelangen changed the title Addon-Docs: Fix scroll to non-ascii anchors Blocks: Fix scroll to non-ascii anchors Aug 14, 2024
@SkReD
Copy link
Contributor Author

SkReD commented Aug 16, 2024

@SkReD Thank you for this PR!

Its a small change, I agree however, I think a manual steps to reproduce should be defined on this PR, as there is also no issue linked. The PR should provide more context, for future us.

Even better would be a story somewhere where we can quickly verify the correct behavior.

story added

@ndelangen ndelangen merged commit 004df0c into storybookjs:next Aug 19, 2024
48 checks passed
@github-actions github-actions bot mentioned this pull request Aug 19, 2024
11 tasks
@JReinhold JReinhold 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 Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants