-
Notifications
You must be signed in to change notification settings - Fork 6
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
Use relative uris for posts. #71
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for blogtorontojscom ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This tripped me up during testing. All links went to the production URL. Makes more sense to make these relative I think!
What command were you using to test it? If you do use
|
src/layouts/BlogPosts.astro
Outdated
@@ -62,7 +62,7 @@ const { | |||
posts.map((post) => ( | |||
<PostCard | |||
variant="large" | |||
url={`${BLOG_URL}${post.url}`} | |||
url={post.url} |
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.
Could you check that post.url
correctly resolves to an absolute url so we don't have inexistant relative urls?
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.
No it's a relative URL, and that's what you want here I think!
But it does resolve and made clicking through the site while testing a lot easier.
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.
My concern is that we have relative URLs like: ./2024/10/some-post
and that will break some pages like the tags pages. For example, if you go to /tags/web-development
and have the relative URL post, it will not work as expected.
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.
Oh no, the URLs start with a slash! So that should be good
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.
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.
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.
Oof sorry, I must have looked at this wrong. Will fix this PR!
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 for checking
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.
No problem, I just fixed the issue on the root. Take a look at this branch: relative-uris...fix/post-urls
It also fixes the issues with the rss feed (turns out it was missing a namespace and the reference to get the posts was wrong)
Fixed! And I also changed a few more instances of this issue |
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.
Let me be very pedantic here: I think it would be better if we fix this at the source, by making post.url
start with a slash than fix in multiple places.
Another option would be to add a url formatting utility, but that may be more complex.
Aside from that it looks good to me, feel free to merge and we can make it more generic in a next PR.
I'm ok with pedantic! I assumed |
It is a built-in, but we are overriding it at the source to include the year and month. I made this change and updated some other places on the branch |
This tripped me up during testing. All links went to the production URL, so it looked like the new page i was adding was 404ing, but I didn't realize I was no longer on the same domain.
Makes more sense to make these relative I think!