FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix jittery scrolling by saltybuckets · Pull Request #40215 · nodejs/node · GitHub

/ node Public

fix jittery scrolling - #40215

Closed
saltybuckets wants to merge 6 commits into
nodejs:mainfrom
saltybuckets:fix-scrolling
ConversationCommits (6)ChecksFiles changed
Closed

fix jittery scrolling#40215
saltybuckets wants to merge 6 commits into
nodejs:mainfrom
saltybuckets:fix-scrolling

Conversation

saltybuckets commented Sep 25, 2021 •
edited by aduh95
Loading

Copy link
Copy Markdown

Subsystem: doc

This pr fixes the jittery scrolling behavior introduced in
#37301

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

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 25, 2021
aduh95 reviewed Sep 26, 2021
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Sep 27, 2021

Copy link
Copy Markdown
Member

@nodejs/website

saltybuckets commented Sep 27, 2021

Copy link
Copy Markdown
Author

@aduh95 done! 👍

saltybuckets requested a review from aduh95 September 27, 2021 04:54
nschonni reviewed Sep 27, 2021
View reviewed changes
Comment thread doc/template.html Outdated
<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>

nschonni Sep 27, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why not add to the existing script?

saltybuckets Sep 27, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

aduh95 reviewed Sep 27, 2021
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you please remove this line:

node/.eslintignore

Line 9 in 8d83c47

doc/**/*.js

And then, run make lint-js-fix.

Comment thread doc/template.html Outdated
Comment thread doc/template.html Outdated
saltybuckets requested a review from aduh95 September 28, 2021 18:14
aduh95 reviewed Sep 28, 2021
View reviewed changes
Comment thread doc/template.html Outdated
Comment thread doc/api_assets/script.js Outdated
Comment thread doc/api_assets/script.js Outdated
Apply suggestions from code review
a6e35c8 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
saltybuckets requested a review from aduh95 September 28, 2021 18:54

saltybuckets commented Oct 4, 2021

Copy link
Copy Markdown
Author

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

aduh95 commented Oct 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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 commented Oct 5, 2021

Copy link
Copy Markdown
Contributor

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

saltybuckets commented Nov 13, 2021

Copy link
Copy Markdown
Author

he didnt reply

Trott commented Nov 14, 2021

Copy link
Copy Markdown
Member

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 commented Nov 14, 2021

Copy link
Copy Markdown
Contributor

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 commented Nov 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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?

aduh95 mentioned this pull request Feb 6, 2022
Closed
dholbert mentioned this pull request May 26, 2023
Merged

marco-ippolito commented Nov 2, 2024

Copy link
Copy Markdown
Member

Superseded by #53510

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 Awaiting requested review from aduh95
1 more reviewer
nschonni nschonni left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

doc: weird scrolling/anchor behavior in Chrome and Firefox

7 participants

Back | FazBrowse Home | New Git URL