-
-
Notifications
You must be signed in to change notification settings - Fork 504
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 last updated dates for pages displaying fallback content #1044
Fix last updated dates for pages displaying fallback content #1044
Conversation
🦋 Changeset detectedLatest commit: 444b438 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks for quickly picking this up @HiDeoo — and especially for adding some testing here. Will help us know if my suggestion actually works or not :-D
const currentId = isFallback ? localizedId(id, config.defaultLocale.locale) : id; | ||
const currentFilePath = fileURLToPath(new URL('src/content/docs/' + currentId, project.root)); |
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.
I actually think we can simplify it to this!
const currentId = isFallback ? localizedId(id, config.defaultLocale.locale) : id; | |
const currentFilePath = fileURLToPath(new URL('src/content/docs/' + currentId, project.root)); | |
const currentFilePath = fileURLToPath(new URL('src/content/docs/' + entry.id, project.root)); |
PageProps
includes a root-level id
localized for the current page, but entry
also includes the original id
for the content being used.
@@ -66,9 +66,10 @@ function getToC({ entry, locale, headings }: PageProps) { | |||
}; | |||
} | |||
|
|||
function getLastUpdated({ entry, id }: PageProps): Date | undefined { | |||
function getLastUpdated({ entry, id, isFallback }: PageProps): Date | undefined { |
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.
And then this is also simpler!
function getLastUpdated({ entry, id, isFallback }: PageProps): Date | undefined { | |
function getLastUpdated({ entry }: PageProps): Date | undefined { |
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Brilliant idea, did not think of that at all and it works perfectly. Thanks a lot! I also slightly improved the test as I was taking a route and "faking" it to be a fallback one instead of just using a fallback route ^^ Not sure what I was thinking there. |
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.
Thanks @HiDeoo — looks great! Should be safe to merge whenever you like 🌟
What kind of changes does this PR include?
Description
This pull request fixes the last updates dates for pages displaying fallback content by taking into account
isFallback
when generating the file path used withgetFileCommitDate()
.Preview: