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

fix: Prevent initial unnecessary IntersectionObserver callback execution #2523

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

sy-records
Copy link
Member

Summary

  • Added a flag (isInitialLoad) to skip the first callback execution of the IntersectionObserver on page load.

Related issue, if any:

Fix #2504
Fix #2509

What kind of change does this PR introduce?

Bugfix

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

No

Yes
No

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge

Copy link

vercel bot commented Jan 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docsify-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2025 11:21am

paulhibbitts
paulhibbitts previously approved these changes Jan 3, 2025
Copy link
Collaborator

@paulhibbitts paulhibbitts left a comment

Choose a reason for hiding this comment

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

Thanks @sy-records , created Sandbox and tested (see issue comments) - all looks good to me!

@sy-records sy-records requested a review from a team January 4, 2025 02:34
Copy link
Member

@jhildenbiddle jhildenbiddle left a comment

Choose a reason for hiding this comment

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

Looks good, @sy-records. A few minor nitpicks.

src/core/event/index.js Outdated Show resolved Hide resolved
src/core/event/index.js Show resolved Hide resolved
src/core/event/index.js Show resolved Hide resolved
Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

Shall we promote the isInitialLoad to the Object level as a field?
Then, we could reuse and keep consistence when any logic relies on the same isInitialLoad lifecycle flag in future.

Copy link
Collaborator

@paulhibbitts paulhibbitts left a comment

Choose a reason for hiding this comment

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

Thanks @sy-records and @jhildenbiddle , I've retested in the sandbox (with new Preview build) and things look good to me!

https://codesandbox.io/p/sandbox/docsify-v5-sidebar-fix-zwdm6p

@sy-records sy-records merged commit a73e07e into docsifyjs:develop Jan 6, 2025
9 checks passed
@sy-records sy-records deleted the fix/sidebar branch January 6, 2025 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants