-
-
Notifications
You must be signed in to change notification settings - Fork 578
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 timezone-reliance in LastUpdated #1170
Conversation
🦋 Changeset detectedLatest commit: 2218038 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hello! Thank you for opening your first PR to Starlight! ✨ Here’s what will happen next:
|
LGTM! Edit: The only thing that is missing is a changeset 👍 |
6a2ddff
to
c576670
Compare
👍 Added a changeset. |
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 the PR @tmcw! And sorry it got buried as we were prepping for Astro v4 and migrating Astro’s Docs to run on Starlight.
Have we tested this works as expected with git-based last updated times as well?
The updated date can come from git or from frontmatter, so ideally it would be great to handle here:
starlight/packages/starlight/utils/route-data.ts
Lines 72 to 84 in 7526059
function getLastUpdated({ entry }: PageProps): Date | undefined { | |
if (entry.data.lastUpdated ?? config.lastUpdated) { | |
const currentFilePath = fileURLToPath(new URL('src/content/docs/' + entry.id, project.root)); | |
let date = typeof entry.data.lastUpdated !== 'boolean' ? entry.data.lastUpdated : undefined; | |
if (!date) { | |
try { | |
({ date } = getFileCommitDate(currentFilePath, 'newest')); | |
} catch {} | |
} | |
return date; | |
} | |
return; | |
} |
This would also mean anyone overriding this component would get the correct behaviour without having to replicate this logic. May not be possible though given we’re providing a Date
object in route data.
I don't think that the issue is that the Date object is wrong. It just displays the wrong time when its converted to a string because it adjusts to the local timezone. I fear that changing the date object upstream might result in unexpected date behavior for users. |
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.
Yeah, you’re probably right @kevinzunigacuellar! Let’s give it a try and if anyone notices broken behaviour with this change, hopefully they’ll let us know — thanks again @tmcw 🙌
* main: (440 commits) i18n(fr): update `getting-started.mdx` (withastro#1245) [ci] format docs(showcase): add csmos.space (withastro#1269) [ci] format docs: add SiteOne Crawler to showcase (withastro#1264) Fix formatting [ci] format docs(i18n): Add Indonesian translation for site search documentation (withastro#1250) i18n(es): fix typo (withastro#1246) i18n(ja): Update getting-started.mdx (withastro#1252) i18n(es): Update `getting-started.mdx` (withastro#1247) i18n(ko-KR): update `getting-started.mdx` (withastro#1248) Update upgrade instructions to show new `@astrojs/upgrade` (withastro#1241) [ci] format [ci] release (withastro#1240) i18n(ru): Fix typo in `i18n.untranslatedContent` (withastro#1243) Fix timezone-reliance in LastUpdated (withastro#1170) Prefetch links on hover by default (withastro#1242) Add support for Astro v4, drop support for Astro v3 (withastro#1238) Add Matrix social icon (withastro#1203) ...
Description