-
Notifications
You must be signed in to change notification settings - Fork 0
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
feature: link to files (for download) #205
Conversation
resolves Downloadable files #148
Deploying head-start with Cloudflare Pages
|
@@ -54,7 +54,6 @@ export default defineConfig({ | |||
output: isPreview ? 'server' : 'static', | |||
server: { port: localhostPort }, | |||
site: siteUrl, | |||
trailingSlash: 'always', |
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.
This is required for the file proxy to work (or otherwise the file href need a /
suffix like dummy.pdf/
) which is odd. I'm okay with having the required trailing slash removed as all our routing and canonicals make sure we have consistent page routes ending with a /
.
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.
Do we have tests in place to verify that trailing slashes for routes to records other than files do in fact work?
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.
Not yet. I see the routing tests only cover redirects: https://github.com/voorhoede/head-start/blob/main/src/lib/routing/index.test.ts#L31
Will add tests
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.
Added tests. See commit 587caf9
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.
Figured I should actually also include tests for the LinkToFile component itself. WIP: 970401f (didn't have enough time to finish it)
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.
Record should be the right type. I think that's of bigger benefit than having the abstraction of the function
@@ -54,7 +54,6 @@ export default defineConfig({ | |||
output: isPreview ? 'server' : 'static', | |||
server: { port: localhostPort }, | |||
site: siteUrl, | |||
trailingSlash: 'always', |
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.
Do we have tests in place to verify that trailing slashes for routes to records other than files do in fact work?
Typescript is otherwise unable to infer record as a FileRouteFragment
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.
Discussed changes and ready to merge
Changes
file
model that provides a generic pattern for content editors to add and reference files.LinkToFile
component. See its README for details./files/[...filename].ts
route to proxy files, so they can be used as downloads.In CMS:
In Page (EN and NL):
Associated issue
Resolves #148
Closes #147 (outdated earlier attempt based on older routing)
How to test
Checklist
I have added a decision log entry if the change affects the architecture or changes a significant technology