-
Notifications
You must be signed in to change notification settings - Fork 664
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
[SITE] Image Rendering #14341
base: main
Are you sure you want to change the base?
[SITE] Image Rendering #14341
Conversation
Signed-off-by: Toshaan Bharvani <[email protected]>
* the event page logos render to 236x236, making all logos square devopsdays#14336 * the welcome page shortcode will render the event logo Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
✅ Deploy Preview for devopsdays-web ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
First pass, this looks good! I need to dig into it on my computer to fully approve but think it is likely a good one! |
I know this PR does not add documentation and I prefer to do that after this PR, it is optional so nothing should break. Just for extra information, relating to #14336 , you can test the square resizing with the Cairo event image on the index page |
{{- if (where (readDir "assets/sponsors/") "Name" $imagefilename) -}} | ||
{{- $imagelocation := (printf "sponsors/%s" $imagefilename) -}} | ||
{{- $imageresource := resources.Get $imagelocation -}} | ||
{{- $imagefile := $imageresource.Fit "600x600 webp Lanczos q100" -}} |
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 don't think we want the sponsor image to be 600x600?
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.
Yes, I have them all mostly at 600x600, this is just to make them fit nicely, we should have them with the correct sizing per type, so the result is smaller in file size
What would be best for the sizing for each of these
- Welcome page : 236x236
- Sponsor logos
- Speaker page
- Program Talk Page
- Organizers Page
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.
Yes, I have them all mostly at 600x600, this is just to make them fit nicely, we should have them with the correct sizing per type, so the result is smaller in file size
What would be best for the sizing for each of these
- Welcome page : 236x236
- Sponsor logos
- Speaker page
- Program Talk Page
- Organizers Page
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.
Welcome page : 236x236 (i think? this might be too small; maybe double it?)
Sponsor logos: (see below)
Speaker page: (assume you mean speaker headshots, these should be 600 x 600)
Program Talk Page: Same as speaker page (600x600)
Organizers Page: Same as speaker page (600x600)
if we wanted to go smaller, the "headshot" type images (speaker, organizer, talk) can all be 300x300 but iirc the way we are doing responsive/retina images, we want them at the 2x size. that said, that could be Old Way of Doing Things (also since it's a build time sizing, we can try it smaller and see how they look and it's non-destructive)
For sponsors, the guidance we have given is:
The dimensions of the image file must be at least 200px wide. 600px will look best for high-density display.
The background must be either white or transparent.
Images do not need to be square, but they may look better if they are.
so I don't think we hard code both height and width, I think we just set it to 600px wide and let height go proportional?
Adding a comment for future implementation; the sizing of various images should be set in the |
Signed-off-by: Toshaan Bharvani <[email protected]>
* the event page logos render to 236x236, making all logos square devopsdays#14336 * the welcome page shortcode will render the event logo Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
okay it's breaking if there is a folder for the event in the example:
|
Signed-off-by: Matty Stratton <[email protected]>
Signed-off-by: Matty Stratton <[email protected]>
A little experimenting... Here is the build performance with the POC images only:
This is the build performance when the full sponsor image directory is used in the
(also random error - To summarize: When it only has to process a few images, the build (on my laptop) takes a little less than 1 minute. When the full sponsor directory is used, it takes 9.5 minutes. I'm doing a test now to see how long it takes on builds when things have not changed (most sponsor images, once generated, will never change) but note: on deploy previews, it is a completely fresh build every time, so our deploy previews will take AT LEAST 10 minutes (probably longer, as the netlify build containers are less powerful than my laptop) and likely will time out. |
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.
We need to address/revisit the build performance implications of this prior to merging, even with the small "opt in" version IMHO
Update: subsequent build was only about 30 seconds faster :/ |
These images don't change. Some possibly heretical ideas to take advantage of that:
|
OK, should we have that added in this PR or create a new linked PR
I have never used any of these scripts, but they are
Well, yes, those would be a good first target and a good test of endurance.
Ok, so my tests are as follows, first number without cache, second number with cache I tested my other Hugo setups and most have image folders of no more than 200 images per folder. We seem to suffer from the folder overload problem, the I will update the branch and run some more tests after which I hope @mattstratton can confirm that the improvement also enhanced the build time.
Unless caching is added, the build time will to speed up, the difference here seems to be resource bound and not actually Hugo doing anything differently.
Well, actually Hugo has this options, I presumed we had the cache already built in, however @mattstratton tests show the cache not being used correctly and we need to optimize for that.
Yes, we need Netlify to build with the cache, I did see some reference in the Hugo and Netlify documentation and I will try to use that.
No, that negated the purpose of using Hugo Image Pipes, and would mean that we add more work for individual event organisers. |
I've thought about the "divide the sponsor folder up by first initial" thing before; my concern is more about how to keep that enforced? I guess we can put some logic into the linter for pull requests, and watch out for it, but I am slightly concerned about people not following this :) it's not necessarily a reason not to do it, but we def need to make sure it's easy to catch doing it the "wrong" way (ask me how I know, after almost ten years of merging PRs here LOL) |
I'm guessing it's this? https://www.netlify.com/integrations/community-built/hugo-cache-resources-build-plugin/ I do wonder about intiial local builds however; remember that lots of folks run this once a year, and even though we have codespaces, gitpod, etc, most folks are building it locally although I guess that cache is also stored in git somehow? So when they clone it down... ...which can make the repo bloat, right? which is also a concern we have hmm
|
i suspect if we want to keep the file cache of the processed assets, then this line should be removed from Line 2 in aa2430f
|
i'm confused about how this is not set correctly? From what I can read, it just works by default. The cache potential values in the output from my tests are about caching partials, not asset resource caching? |
{{- if (where (readDir "static/events") "Name" $e.name) -}} | ||
{{- $.Scratch.Set "assetsdir" (printf "assets/events/%s/" $e.name) -}} | ||
{{- if (where (readDir "assets/events") "Name" $e.name) -}} | ||
{{- $imagelocation := (printf "events/%s/logo.png" $e.name) -}} |
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.
if you look below, you'll see that we support the logo being JPG as well. this bails if it looks for .png and finds .jpg instead. at minimum we should put in a check for the .png file existing (bc the assets dir might be there but using a jpg version)
If we want a cheap and easy way to save a few gigs today, we never (to my knowledge) did the follow-up on #13018, which was to run BFG9000 on the repo to eliminate all of the now-removed historical images from the repository. I'll make a scientific wild-ass guess that cleaning up the repo, then committing the entire site's worth of cache, would still be smaller than the current state. |
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
* add caching to prevent rebuilding everything everytime * enable stats so we can understand the build speed Signed-off-by: Toshaan Bharvani <[email protected]>
Yes, I understand the concern, and I think while the number of files was smaller, it would be an unnecessary overhead, however, at present I think we need to find a way to solve this.
I also understand that now much better, and I think adding a check for that is the only way
Yes, that and maybe also enable it in the deploy script
No, I do not think we should do that, HugoIO creates the caches locally and these should not be in the git repository.
Unless we want a cache repository as a submodule that gets updated once in a while, while the embedded folders remains empty, or a artifact repository you can download with the cached data.
|
To be clear, I am not saying this is a reason not to divvy it up, I am very much in favor of it for a few reasons! Just was thinking though the things we would want to account for (vs saying it as a reason not to do it!)
you don't even really enable it in a script, per se, it's just a netlify plugin that goes in the config - easy!
Definitely no on submodules! I think that if splitting this up means a local build still runs reasonably, then it's a non issue. I was confused about the "cache configuration" but I understand a few things better after seeing the latest stuff to this PR, thanks! I realize this is a LOT of back and forth on this PR, but this is really a huge (and important!) improvement and I'm glad we are spending the time on getting it right - that's awesome! |
* move all static sponsor images to the assets folder * alphabetize the sorting to use the first letter * remove several images that generate problems - lenovo.png - oracle-mysql.png - netlabs.png - igv.png - nexa.png - universidad-de-montevideo.png - montevideocomm.png - idatha.png there may be more, but this is the list I found Signed-off-by: Toshaan Bharvani <[email protected]>
images not copied from static * dasa.png * devops-meetup-capetown.png Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Signed-off-by: Toshaan Bharvani <[email protected]>
Well, it has been done now, so lets see how this works out over time. At present with the alpabetised folders builds between 35 s and 50 s (cached) on my laptop and about 2m30s in Netlify
Well, I did it in the config, not sure if it is working, but you can add it to the
OK, so a "full" build without any
Yes, no hurry or worry, we need to check every aspect and that takes times, especially so we do not break anything. |
I'm doing a little bit of testing here, and had a couple of thoughts:
|
This PR renders all images to webp using
hugo
extended featuresThe codes enables images within the
assets/
folder to be rendered, while mainting the existingstatic/
folders.It converts any image format into webp using compression, and resizing the image.
The following images are rendering
To prove the pages work, I have added 2024-antwerp examples, proving that a mix of
static/
andassets
folders work, with a preference of using theassets/
overstatic/