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 jittery scrolling #40215

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

saltybuckets
Copy link

@saltybuckets saltybuckets commented Sep 25, 2021

Subsystem: doc

This pr fixes the jittery scrolling behavior introduced in
#37301

Fixes: #40099
Refs: https://infrequently.org/2020/12/resize-resilient-deferred-rendering/

This pr fixes the jittery scrolling behavior introduced in
nodejs#37301

fix:nodejs#40099
refs:https://infrequently.org/2020/12/resize-resilient-deferred-rendering/
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 25, 2021
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Thanks for sending this. Can you please revert the unrelated stylistic changes? Also it'd be nice if the JavaScript code was using the same stylistic choices as the rest of the codebase – maybe you can put this to a separate .js file in doc/api_assets so the linter can autofix that for you.

@Trott
Copy link
Member

Trott commented Sep 27, 2021

@nodejs/website

@saltybuckets
Copy link
Author

@aduh95 done! 👍

@@ -9,6 +9,7 @@
<link rel="stylesheet" href="assets/style.css">
<link rel="stylesheet" href="assets/hljs.css">
<link rel="canonical" href="https://nodejs.org/api/__FILENAME__.html">
<script src="assets/script.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Why not add to the existing script?

Copy link
Author

Choose a reason for hiding this comment

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

@aduh95 asked to move it to a different file

maybe you can put this to a separate .js file in doc/api_assets so the linter can autofix that for you.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you please remove this line:

doc/**/*.js

And then, run make lint-js-fix.

doc/template.html Outdated Show resolved Hide resolved
doc/template.html Outdated Show resolved Hide resolved
doc/template.html Outdated Show resolved Hide resolved
doc/api_assets/script.js Outdated Show resolved Hide resolved
doc/api_assets/script.js Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@saltybuckets
Copy link
Author

@nschonni can you review the changes pls since @aduh95 has been gone for 6 days

@aduh95
Copy link
Contributor

aduh95 commented Oct 5, 2021

@Sanchitverma78 I understand it's frustrating when a PR gets stalled because of a lack of reviews, please also understand that most (all?) Node.js contributors are doing reviews on their spare time, so you have to understand that sometimes things takes time because of a lack of volunteers and different priorities.
It's of course OK for you to ask for reviews, you may ping me to ask me when I'll be available to review your PR, you may also ping other collaborators, but tagging me to at the third person feels a bit aggressive and unnecessary so please don't.

@aduh95
Copy link
Contributor

aduh95 commented Oct 5, 2021

@mscdex Are you able to confirm this is fixing the issue you were experiencing?

@saltybuckets
Copy link
Author

he didnt reply

@Trott
Copy link
Member

Trott commented Nov 14, 2021

I built with this just now and it doesn't fix the jittery scrolling for me, I'm sorry to say. Is there anything I can do to make this feedback more helpful? Like, would a screen recording help or something like that?

@mscdex
Copy link
Contributor

mscdex commented Nov 14, 2021

I haven't tested this myself yet. Assuming this solution should fix this particular problem, I think there are a few things that probably need adjusting compared to the code used at the referenced article:

  • The rootMargin values are probably tied to/related to the values used in the default contain-intrinsic-size values.
  • I think the query selector being used currently is going to be way too restrictive. I would guess all elements in the (right-hand side) content area should be selected since they all scroll together.
  • The referenced article mentions making the script inlined at the bottom of the document whereas we're currently using defer async, I'm not sure if that's important or not.

@mscdex
Copy link
Contributor

mscdex commented Nov 14, 2021

Additionally, it's possible that the current contain-intrinsic-size values in the docs css need to be adjusted for the script to work properly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: weird scrolling behavior in Chrome
6 participants