Fix go to top [Fixes #15412]#15439
Conversation
smithrashell
requested review from
corwintines,
minimalsm,
pettinarip and
wackerow
as code owners
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
wackerow
left a comment
wackerow
left a comment
There was a problem hiding this comment.
Hey @smithrashell! Thanks for the fix! This does work, but I think we can take it another step to make sure we don't break any html rules, and simplify the need to add id's.
Let me know if you have issues!
| */} | ||
| <SkipLink /> | ||
| <div className="mx-auto max-w-screen-2xl"> | ||
| <div className="mx-auto max-w-screen-2xl" id="top"> |
There was a problem hiding this comment.
Want to be a little careful here since this same id is already being used on some pages, such as the Docs pages, and this would result in them being duplicated which isn't valid HTML.. I think we can do this another way.
| variant="outline" | ||
| isSecondary | ||
| onClick={() => scrollIntoView("__next")} | ||
| onClick={() => scrollIntoView("top")} |
There was a problem hiding this comment.
This function currently accepts an id (toId), but we could broaden it's usage by refactoring to use a css selector string with querySelector
I would just update scrollIntoView to accept a query selector instead of an id, and then we can just target the h1 in this case, and update any other instances passing an id to prefix a #
For example:
// src/lib/utils/scrollIntoView.ts
export const scrollIntoView = (
selector: string,
options: ScrollIntoViewOptions = { behavior: "smooth", block: "start" }
): void => {
const element = document.querySelector(selector)
if (!element) return
element.scrollIntoView(options)
}Then this line becomes
| onClick={() => scrollIntoView("top")} | |
| onClick={() => scrollIntoView("h1")} |
And I believe the only other place we use this is in Button, where we can prefix the id with #:
// src/components/ui/buttons/Button.tsx:108
toId && scrollIntoView("#" + toId)There was a problem hiding this comment.
Hi @wackerow , Thanks for the clear feedback! I've updated the implementation as suggested and just pushed the changes. Let me know if anything else needs adjustment.
wackerow
left a comment
wackerow
left a comment
There was a problem hiding this comment.
This looks great! Thanks so much @smithrashell... working great for me, pulling in!
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2025 Ethereum.org Contributor: Head to gitpoap.io & connect your GitHub account to mint!Keep buidling, keep learning, and let's grow the Ethereum open-source community together 🌱 Learn more about GitPOAPs here. |
|
@all-contributors please add @smithrashell for bug fix |
|
I've put up a pull request to add @smithrashell! 🎉 |
2 participants
Description
The "Go to top" button was not functioning due to a missing anchor target.
This PR:
id="top"to themaincontainer inBaseLayout.tsxhrefused by the "Go to top" button inFooter.tsxto point to#topRelated Issue
Fixes #15412