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 mailing list link bug #344

Closed
wants to merge 1 commit into from
Closed

Fix mailing list link bug #344

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/src/components/mailing-list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import MailingListForm from './forms/mailing-list';

export default function MailingList() {
return (
<section id="mailing-list" className="relative bg-keystatic-highlight">
<section className="relative bg-keystatic-highlight">
{/* Using this hidden link while there is a Next 13 issue for # links: https://github.com/vercel/next.js/issues/44295 */}
<a id="mailing-list" aria-hidden="true" className="sr-only" />
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me when going to https://keystatic-site-6sga3atq0-thinkmill-labs.vercel.app/#mailing-list this still doesn't work, although when a try on localhost:3000/#mailing-list it does (the first time)

It's strange, because https://keystatic-site-6sga3atq0-thinkmill-labs.vercel.app/docs/collections-and-singletons#singletons for example seem to work as intended 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it is something to do with the absolute positioning or hidden classes. 🤔

Any reason we shouldn't just wrap the h2 with an anchor? Offset is the only reason I can think of

Copy link
Contributor Author

@ChrisLaneAU ChrisLaneAU Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Interestingly, the way I tested was to go to https://keystatic-site-6sga3atq0-thinkmill-labs.vercel.app and modify the url to add /#mailing-list. This worked. Refreshing from that point also worked. What I didn't test was scrolling back to the top of the page and then refreshing. That doesn't work. Or building the link and clicking on it from an external source.

If you have the element in the viewport, then all good, refresh works. Even if you have the element near the viewport it will at least attempt a scroll. The same process (url mod, refresh at different scroll points) works for keystatic.com 😅

Any reason we shouldn't just wrap the h2 with an anchor

This also has the same result.

Short of a custom hook, I think the best options are:

  1. Create a /mailing-list page to hold that component that we link to from the README and other external sources
  2. Leave it as-is and wait for Next to implement a fix

It's only partly broken, you are at least taken to the correct page when you click on the /#mailing-list link.

Open to thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's only partly broken, you are at least taken to the correct page when you click on the /#mailing-list link.

I'd prefer it not to be broken at all.

This problem is actually a bit deeper than we thought. I couldn't understand why this wasn't working as it is standard browser behaviour, was NextJS overriding the browser behaviour? Why was it working on our other docs pages but not this one?

Turns out this is an issue with our homepage. Unlike the doc content pages where this works, the homepage isn't being server rendered at all. When viewing the source none of the homepage markup is there, so we need to fix that first.

I'll take a look.

<svg
className="absolute inset-x-0 bottom-0"
xmlns="http://www.w3.org/2000/svg"
Expand Down
Loading