-
Notifications
You must be signed in to change notification settings - Fork 13
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
GitHub Action to Generate Website and Commit to Repository #34
base: main
Are you sure you want to change the base?
Conversation
to generate website using optimized images
const files = text.split(/\r?\n/i) | ||
files.forEach(addImg) | ||
}) | ||
.catch(console.error) |
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.
Is it possible to add more context toconsole.error
, i.e. make the log message more meaningful?
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 am looking into this.
-ms-flex: auto; | ||
flex: auto; | ||
width: 200px; | ||
margin: .5vw; |
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.
Is it possible to move the styles to an external CSS file?
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.
The final UI is going to come from #8 which is why I thought it is not necessary to move the CSS to another final.
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.
@saminarp fair enough.
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 creating this PR 👍🏼
The overall demo is looking good. I have requested some minor changes.
Thank you @SerpentBytes for reviewing this PR! |
gallery.appendChild(division) | ||
} | ||
|
||
fetch("./filelist.txt") |
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 technique is fine as a first pass, but we don't need to dynamically load the file list and build the DOM, since we already have a build step (i.e., GitHub Actions) and can generate the static HTML with all images before we commit the file to git. This will give a much better experience to the user, since the initial load will be faster, and the images can be properly positioned and sized (i.e., our build step can also figure this out, and add intrinsic sizes).
If you want to leave this optimization for later, that's fine. File a follow-up issue.
SRC_FOLDER_PATH: 'optimized' | ||
TARGET_BRANCH: 'gh-pages' | ||
run: | | ||
files=$(find $SRC_FOLDER_PATH -type f) # get the file list |
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 should be moved to a shell script in the repo, and run as a single command from there.
Fixes #5
This GitHub Action workflow takes the images uploaded from
optimized
directory and generates a website using GitHub pages. See the example of generated website from this linkPreview of the website
Once this gets approved, we need to replace the branch name (
issue#5-gen-web
) inpublish.yml
tomain
To test
You can change your branch to
issue#5-gen-web
and upload your desired pictures tooptimized
directory. After a successful commit and a push, this branch will result in a gh-pages website.