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.

Sign up for GitHub

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

Jump to bottom

doc: remove incorrect scroll-padding-top offset #53679

Closed
wants to merge 1 commit into from
Closed

doc: remove incorrect scroll-padding-top offset #53679

wants to merge 1 commit into from

Conversation

avivkeller
Copy link
Member

Fixes #53594

This PR modifies the behavior of the docs to not begin scrolling halfway through the screen, but rather at the browser default.

Before

screen-capture.2.webm

After

screen-capture.4.webm

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/nodejs-website

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 1, 2024
@ovflowd
Copy link
Member

ovflowd commented Jul 2, 2024

The title is erroneous, no?

@avivkeller
Copy link
Member Author

avivkeller commented Jul 2, 2024 •
edited
Loading

I don't think so, this PR updates the scrolling speed to be slower than what it was, what do you recommend for the commit?

@ovflowd
Copy link
Member

ovflowd commented Jul 2, 2024

No? The title should be: "doc: removing wrong scroll padding"

@avivkeller
Copy link
Member Author

Got it, thanks! I'll fix it now!

@avivkeller avivkeller changed the title doc: fix scrolling speed doc: remove incorrect scroll-padding-top offset Jul 2, 2024
@ovflowd
Copy link
Member

ovflowd commented Jul 2, 2024

I guess I understand now what that other PR was, but isn't this a duplicated PR then?

@avivkeller
Copy link
Member Author

avivkeller commented Jul 2, 2024 •
edited
Loading

I guess I understand now what that other PR was, but isn't this a duplicated PR then?

What other PR? The linked item is an issue. It was describing a behavior that this PR fixes, but the way they worded the behavior was a bit odd IMO.

@ovflowd
Copy link
Member

ovflowd commented Jul 2, 2024

Ah interesting! I thought the same author made a PR reverting their changes that got merged back a few while ago or something like that. Too many notifications, I might be seeing things.

@jakecastelli
Copy link
Member

jakecastelli commented Jul 2, 2024 •
edited
Loading

I think you might be correct about the other PR @ovflowd, do you mean this one?

kudos to both of you for looking into and fixing this annoying bug, I literally gave up on selecting text on the doc page and copied directly from the source code 😄

@avivkeller
Copy link
Member Author

avivkeller commented Jul 2, 2024 •
edited
Loading

Ha! I didn't see that PR, closing in favor.

Great minds think alike (and apparently time alike too)

@avivkeller avivkeller closed this Jul 2, 2024
@avivkeller avivkeller deleted the patch-65 branch July 2, 2024 02:19
@avivkeller avivkeller mentioned this pull request Jul 2, 2024
Merged
@avivkeller avivkeller added the duplicate Issues and PRs that are duplicates of other issues or PRs. label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. duplicate Issues and PRs that are duplicates of other issues or PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

doc: remove scroll up when highlighting text
4 participants
@avivkeller @nodejs-github-bot @ovflowd @jakecastelli