-
Notifications
You must be signed in to change notification settings - Fork 2
rebuild thumbnail sizes file in build action #26
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds a dedicated script to generate a JSON file of thumbnail dimensions, removes JSON logic from the existing image script, and integrates the new script into the CI workflow.
- Introduce
_scripts/thumbnail_sizes.sh
to scan thumbnails and emitthumbs.json
. - Strip JSON handling out of
_scripts/img.sh
. - Update
.github/workflows/pages.yml
to run the new script and auto-commit its output.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
_scripts/thumbnail_sizes.sh | New script to collect thumbnail dimensions into a JSON file |
_scripts/img.sh | Removed inline thumbnail JSON generation |
.github/workflows/pages.yml | Added workflow steps to run thumbnail script and commit changes |
Comments suppressed due to low confidence (3)
_scripts/thumbnail_sizes.sh:14
- The
\n
in the format string adds a newline into the variable, which will break JSON formatting. Use-format "%w,%h"
(no newline) and append newlines only viaecho
orprintf
.
dimensions=$(magick identify -format "%w,%h\n" "$thumb")
.github/workflows/pages.yml:53
- Pushing commits on a
push
event will trigger the workflow again, causing a loop. Add a condition to skip runs when the actor is GitHub Actions or include[ci skip]
in the commit message.
- name: 🚀 Commit and push changes
.github/workflows/pages.yml:1
- [nitpick] The job name
Build artifacts
no longer reflects building or deploying pages. Consider renaming to something likeGenerate and Commit Thumbnails
for clarity.
name: Build artifacts
@alexwolson this PR is a draft, I'll mark it as ready for review once it's ready for review |
For reference: I implemented a couple gems on CivicTechTO/civic-spark.com#8 that may be applicable to our problem space. |
Description
Extracted the thumbnail sizes functionality into its own shell script and added it to the build action.
How to Test
Since the build action runs on pushes to
main
, to test this we can either temporarily change this and deploy from this branch, or merge this PR tomain
and then test it.Now, when we add a JPG image to
assets/images/hacknights/thumbnails
, the build script should include its data in the generated thumbs.json file. The JPG image should be a different aspect ratio than square and different dimensions than 300x300, because those are the defaults:civictech.ca/assets/js/lazy-image.js
Lines 37 to 39 in 09e1484
Notes
We aren’t committing any of the generated artifacts from the build script back into the repo, which is fine. doing so could create an infinite loops. the only downside is if you run the site locally, you won't have the most up to date build unless you manually run the build command, which will create a bunch of changes in the repo.
It would be ideal to not commit any of the generated artifacts to the repo, as they can be derived using the build script. You’d just need to run the build script locally before running the server for the first time.
To do