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

fix(gatsby-react-router-scroll): replace querySelector with getElementById #25161

Merged
merged 2 commits into from
Jun 22, 2020
Merged

fix(gatsby-react-router-scroll): replace querySelector with getElementById #25161

merged 2 commits into from
Jun 22, 2020

Conversation

hartshorne
Copy link
Contributor

Description

@blainekasten introduced a number of improvements to scroll handling in #24306.

In the scrollToHash function, we should use document.getElementById to find the correct node instead of document.querySelector.

The HTML standard allows almost any string to be used as an id attribute:

There are no other restrictions on what form an ID can take; in particular, IDs can consist of just digits, start with a digit, start with an underscore, consist of just punctuation, etc.

From: https://html.spec.whatwg.org/multipage/dom.html#the-id-attribute

document.querySelector expects a valid CSS selector string, and will throw an exception if you do not supply one:
https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector

document.getElementById is designed to be used with the HTML id attribute: https://developer.mozilla.org/en-US/docs/Web/API/Document/getElementById

And does not seem to throw any exceptions, even if you give it invalid input.

document.getElementById also works with non-ascii text, like the áccentuated mentioned in #24306.

And for bonus points, it looks like document.getElementById is about twice as fast as document.querySelector:
https://www.measurethat.net/Benchmarks/Show/2488/0/getelementbyid-vs-queryselector

Documentation

None.

Related Issues

None.

@hartshorne hartshorne requested a review from a team as a code owner June 20, 2020 20:51
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 20, 2020
@wardpeet
Copy link
Contributor

@hartshorne could you add a test case for this?

@hartshorne
Copy link
Contributor Author

hartshorne commented Jun 20, 2020 •
edited
Loading

HTML:

<html>
  <h1 id="hello=world">Hello, world</h1>`
</html>

JS:

document.querySelector("#hello=world"); // throws
document.getElementById("hello=world"); // returns <h1> node

Or are you looking for something with the Gatsby environment wrapped around it? Or are you looking for unit tests?

@hartshorne
Copy link
Contributor Author

@wardpeet I missed the fact that hash would be prefixed with # to work with querySelector.

@freiksenet freiksenet added status: awaiting author response Additional information has been requested from the author status: inkteam assigned and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 22, 2020
blainekasten
blainekasten approved these changes Jun 22, 2020
View reviewed changes
Copy link
Contributor

@blainekasten blainekasten left a comment

Choose a reason for hiding this comment

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

No need for unit tests on this, we have e2e tests for this so if it passes we're good! Also this makes a lot of sense. Thank you!

@blainekasten blainekasten added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jun 22, 2020
@gatsbybot gatsbybot merged commit 40724e8 into gatsbyjs:master Jun 22, 2020
@gatsbot
Copy link

gatsbot bot commented Jun 22, 2020

Holy buckets, @hartshorne — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@jeesunikim jeesunikim mentioned this pull request Jun 22, 2020
Closed
@nkuehn nkuehn mentioned this pull request Jun 23, 2020
Closed
@blainekasten blainekasten mentioned this pull request Jun 23, 2020
Closed
@spences10 spences10 mentioned this pull request Jul 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blainekasten blainekasten blainekasten approved these changes

Assignees

@blainekasten blainekasten

Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes status: awaiting author response Additional information has been requested from the author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hartshorne @wardpeet @blainekasten @freiksenet @gatsbybot