-
-
Notifications
You must be signed in to change notification settings - Fork 560
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.
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
Fix SSG not generating the actual page content on initial load #1599
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, abhijithmuthyala, for creating this pull request and contributing to LinksHub! 💗
The maintainers will review this Pull Request and provide feedback as soon as possible! 😇
We appreciate your patience and contribution, Keep up the great work! 😀
@abhijithmuthyala |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@CBID2 |
Let's see what @rupali-codes thinks. |
|
So...do you agree or not @rupali-codes? |
i do..remove the labels for now |
Video posted in the linked issue is the bug and the one in this PR is the fix. I thought it would be unnecessary to duplicate the bug video both in the issue and in the PR. If it is preferred to have it in both the places, I'll keep that in mind for future contributions. |
No, you shouldn't add a duplicate video. You can simply reference it. I asked what the change was. It would be helpful to see it visually in the video. |
Hmm, I already posted the bug video in the issue and the fix video in the PR though? Anyways, if it is helpful to repeat the bug video here, sure. I updated the PR description, please check. |
I thought the PR one was the bug video. My bad😅 That's why I was suggesting that you post a video solution, but without repeating the bug video. No worries. I will check it out soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Looks good to me.
It will be better if you can also describe the approach of the code from next time. Because reviewing it without that would take a lot of time.
@rupali-codes
Please review this.
Fixes Issue
Closes #1477
Changes proposed
/
and/[subcategory]
routes), not just theSpinner />
.404
for the/[subcategoryThatsNotInTheDatabase]
routes.Screenshots
Before
empty-prerender.-.Made.with.Clipchamp.mp4
After
ssg.mp4
Note to reviewers
Currently, the route for a subcategory that isn't in the database (
/cat
for example) behaves almost as if a search query with that name is submitted (/search?query=cat
), which certainly is a bug. In this PR I had to choose a fallback, and I went with a404
page (fallback: false
). If it's preferred to retain the current behaviour,fallback: 'blocking'
can be temporarily used.