Skip to content
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

Merged
merged 33 commits into from
Sep 24, 2024
Merged

[SITE] Image Rendering #14341

merged 33 commits into from
Sep 24, 2024

Conversation

toshywoshy
Copy link
Contributor

This PR renders all images to webp using hugo extended features
The codes enables images within the assets/ folder to be rendered, while mainting the existing static/ 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/ and assets folders work, with a preference of using the assets/ over static/

Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
* 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 <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
@toshywoshy toshywoshy requested review from a team as code owners July 13, 2024 23:42
Copy link

netlify bot commented Jul 13, 2024

Deploy Preview for devopsdays-web ready!

Name Link
🔨 Latest commit 38bfb36
🔍 Latest deploy log https://app.netlify.com/sites/devopsdays-web/deploys/66f31d4195873b0008886e8b
😎 Deploy Preview https://deploy-preview-14341--devopsdays-web.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
@mattstratton
Copy link
Member

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!

@toshywoshy
Copy link
Contributor Author

I know this PR does not add documentation and I prefer to do that after this PR, it is optional so nothing should break.
Only if you use the assets/events/<year>-<city>/ does it look at those folders.

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" -}}
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

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?

@mattstratton
Copy link
Member

Adding a comment for future implementation; the sizing of various images should be set in the config/_default/hugo.yml instead of being hardcoded, but we can add that later!

@mattstratton mattstratton requested a review from a team as a code owner September 13, 2024 18:28
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
* 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 <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
@mattstratton
Copy link
Member

okay it's breaking if there is a folder for the event in the assets directory, but none of the subfolders

example:

ERROR render of "page" failed: "/Users/matty.stratton/src/github.com/devopsdays/devopsdays-web/themes/devopsdays-theme/layouts/speaker/single.html:48:23": execute of template failed: template: speaker/single.html:48:23: executing "main" at <readDir $assetspeakersfolder>: error calling readDir: failed to read directory "assets/events/2024-cairo/speakers/": open /Users/matty.stratton/src/github.com/devopsdays/devopsdays-web/assets/events/2024-cairo/speakers: no such file or directory
ERROR render of "page" failed: "/Users/matty.stratton/src/github.com/devopsdays/devopsdays-web/themes/devopsdays-theme/layouts/speakers/single.html:17:31": execute of template failed: template: speakers/single.html:17:31: executing "main" at <readDir $assetspeakersfolder>: error calling readDir: failed to read directory "assets/events/2024-cairo/speakers/": open /Users/matty.stratton/src/github.com/devopsdays/devopsdays-web/assets/events/2024-cairo/speakers: no such file or directory
ERROR render of "page" failed: "/Users/matty.stratton/src/github.com/devopsdays/devopsdays-web/themes/devopsdays-theme/layouts/talk/single.html:155:29": execute of template failed: template: talk/single.html:155:29: executing "main" at <readDir $assetspeakersfolder>: error calling readDir: failed to read directory "assets/events/2024-organiser-summit/speakers/": open /Users/matty.stratton/src/github.com/devopsdays/devopsdays-web/assets/events/2024-organiser-summit/speakers: no such file or directory
Built in 17048 ms
Error: error building site: render: failed to render pages: render of "page" failed: "/Users/matty.stratton/src/github.com/devopsdays/devopsdays-web/themes/devopsdays-theme/layouts/speaker/single.html:48:23": execute of template failed: template: speaker/single.html:48:23: executing "main" at <readDir $assetspeakersfolder>: error calling readDir: failed to read directory "assets/events/2024-cairo/speakers/": open /Users/matty.stratton/src/github.com/devopsdays/devopsdays-web/assets/events/2024-cairo/speakers: no such file or directory

Signed-off-by: Matty Stratton <matty.stratton@aiven.io>
Signed-off-by: Matty Stratton <matty.stratton@aiven.io>
@mattstratton
Copy link
Member

mattstratton commented Sep 17, 2024

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.

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

Netlify
Persist Hugo resources folder between Netlify builds for huge build speed improvements!

@mattstratton
Copy link
Member

i suspect if we want to keep the file cache of the processed assets, then this line should be removed from .gitignore (I added it the other day but it's not going to be causing any of the current issues with local builds)

/resources/_gen

@mattstratton
Copy link
Member

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.

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) -}}
Copy link
Member

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)

@don-code
Copy link
Contributor

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

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 <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
* add caching to prevent rebuilding everything everytime
* enable stats so we can understand the build speed

Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
@toshywoshy toshywoshy requested a review from a team as a code owner September 17, 2024 14:22
@toshywoshy
Copy link
Contributor Author

toshywoshy commented Sep 17, 2024

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 :)

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.

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 also understand that now much better, and I think adding a check for that is the only way
At present I am more concerned with the correct splitting and after splitting that we actually get improvements.

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.

I'm guessing it's this?

https://www.netlify.com/integrations/community-built/hugo-cache-resources-build-plugin/

Yes, that and maybe also enable it in the deploy script

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...

No, I do not think we should do that, HugoIO creates the caches locally and these should not be in the git repository.

...which can make the repo bloat, right? which is also a concern we have

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.
We could even have that in the build scripts to download the latest cache.
I am not in favour of creating more bloat, niether of having cache in a main repository.

Netlify
Persist Hugo resources folder between Netlify builds for huge build speed improvements!

@mattstratton
Copy link
Member

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.

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!)

Yes, that and maybe also enable it in the deploy script

you don't even really enable it in a script, per se, it's just a netlify plugin that goes in the config - easy!

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.
We could even have that in the build scripts to download the latest cache.
I am not in favour of creating more bloat, niether of having cache in a main repository.

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 <toshaan@vantosh.com>
images not copied from static
* dasa.png
* devops-meetup-capetown.png

Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
Signed-off-by: Toshaan Bharvani <toshaan@vantosh.com>
@toshywoshy
Copy link
Contributor Author

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.

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!)

Well, it has been done now, so lets see how this works out over time.
The logic takes the first letter of the sponsor and uses that in the folder index

At present with the alpabetised folders builds between 35 s and 50 s (cached) on my laptop and about 2m30s in Netlify

Yes, that and maybe also enable it in the deploy script

you don't even really enable it in a script, per se, it's just a netlify plugin that goes in the config - easy!

Well, I did it in the config, not sure if it is working, but you can add it to the netlify.tomlfile as a plugin.
Again not sure if that works without you having to enable it in the GUI or as an admin
From the numbers I suspect the cache is not (yet) working.

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.
We could even have that in the build scripts to download the latest cache.
I am not in favour of creating more bloat, niether of having cache in a main repository.

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!

OK, so a "full" build without any resources/ takes about 1m30s, while a subsequent build with cache takes about 35s.
This is on my laptop running a make file to run builds.
I am sure we can improve that over time, but these numbers seem acceptable enought to me

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!

Yes, no hurry or worry, we need to check every aspect and that takes times, especially so we do not break anything.

@mattstratton
Copy link
Member

I'm doing a little bit of testing here, and had a couple of thoughts:

  1. i think that if this passes for the sponsor directory (all signs point to yes!) we should adjust the sponsors shortcode, etc, to only use the assets directory. I don't think there is any reason to make that one an "opt in".
  2. we cannot remove the /static/img/sponsors directory unfortunately, because too many of the previous "archived to static" pages will have paths to those sponsor images and will break. I think that we can clean that up later and at some point remove the static dir for the sponsors (alternate idea - move them to the assets repo and write a redirect from www to assets for antyhign looking for devopsdays.org/img/sponsors/* etc)

@mattstratton
Copy link
Member

I'm just waiting for this PR to finish the final netlify check to make sure it still all runs fine; i do want to update the sponsor stuff so that the assets directory isn't "opt in" for the sponsors; this will require some updates to the sponsor scripts as well as the documentation (and an update to the sponsor shortcode) but that can come in a follow up PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants