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

adds enforcement of asset directory for square logos #14636

Merged
merged 13 commits into from
Oct 3, 2024

Conversation

mattstratton
Copy link
Member

@mattstratton mattstratton commented Sep 30, 2024

Fixes #14336

This change does the following: (updated description after 3e7da89)

  1. if the file /assets/events/images/YYYY-CITY/logo-square.png exists, it crops it to 236x236 square WebP image for the front page
  2. if the file doesn't exist, but /assets/events/images/YYYY-CITY/logo.png does, it crops it to 236x236 square WebP image for the front page (and logs a warning in the hugo build)
  3. if neither of those is true, but static/events/YYYY-CITY/logo-square.jpg does, it just uses that as the frontpage image
  4. if none of those are true, it uses the default image

(numbers 3 and 4 are not new functionality)

also in this change was to copy the logo-square.jpg files from static for events currently on the home page as a test/demonstration and change them to logo-square.png. The associated static directory version of the files is removed in this PR.

Check out the deploy preview to see the crops; most of them look okay and a couple might need minor changes to the source file (which we can do without the event needing to do it; they are all mostly square already) I fixed the one that seemed to need to be changed to be more square.

Once we feel good about the preview, we can update (in this same PR probably?) to remove step 3, so the static directory will no longer work for front page images.

Copy link

netlify bot commented Sep 30, 2024

Deploy Preview for devopsdays-web ready!

Name Link
🔨 Latest commit 633edfb
🔍 Latest deploy log https://app.netlify.com/sites/devopsdays-web/deploys/66fdf1dea73479000802d19e
😎 Deploy Preview https://deploy-preview-14636--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.

Copy link

Error: Mixed case filenames found

This can cause problems because git and Linux are case sensistive. Please rename the following to lowercase-with-dashes:

Actual Path Suggested Valid Path
assets/events/OLD2024-joao-pessoa assets/events/old2024-joao-pessoa

Please rename the files, commit and push again.

@mattstratton
Copy link
Member Author

Whoops - I was troubleshooting and renamed a directory which fails the linter. Deploy preview worked though. I'll fix it.

@jerdog
Copy link
Contributor

jerdog commented Sep 30, 2024

For easy comparison:

Without the crop:
image

With the crop (and image substitution):
image

@jerdog
Copy link
Contributor

jerdog commented Sep 30, 2024

I think this looks to work right, and it might force some to re-evaluate their image if it doesn't look right after the forced crop

@toshywoshy
Copy link
Contributor

toshywoshy commented Sep 30, 2024

1. if the file `/assets/events/images/YYYY-CITY/logo-square.jpg` exists, it crops it to 236x236 square WebP image for the front page

2. if the file doesn't exist, but `/assets/events/images/YYYY-CITY/logo.png` does,  it crops it to 236x236 square WebP image for the front page (and logs a warning in the hugo build)

Why do we want a logo-square.jpg and a logo.png?
While the logo may only be about 2M, having the same logo twice, for 100 events is about 200M additional storage for an image that can be rendered to become square from the existing logo.png.
I understand that historically we wanted the 2 images and that without image rendering the image cannot be made square, however now that is not relevant.
Also with a PNG we can control the transparency to background conversion if needed

@mattstratton
Copy link
Member Author

Because the logo, which is used by the shortcode on the welcome page etc, does NOT have to be square. The logo square is the option for when you want them different.

If you want to use the same for both front page and for other use, you just use logo.png. Think of logo-square file as the override

we don’t have to worry about “historical” bc the logo square is not used anywhere but the homepage for upcoming events.

@mattstratton
Copy link
Member Author

But I’m okay with making the standard for `logo-square” to be PNG :)

@toshywoshy
Copy link
Contributor

Because the logo, which is used by the shortcode on the welcome page etc, does NOT have to be square. The logo square is the option for when you want them different.

But the renderer 'squares' the image, or are we worried about pixalation?

If you want to use the same for both front page and for other use, you just use logo.png. Think of logo-square file as the override

So it would be optional to have this file.

we don’t have to worry about “historical” bc the logo square is not used anywhere but the homepage for upcoming events.

OK, should we think about clean up?
Removing these square images over time, might remove again several 100MB from the repository.

But I’m okay with making the standard for `logo-square” to be PNG :)

I would prefer the PNG with transparancy, but I am not sure how we can enforce that, otherwise it does not make a big difference

@mattstratton
Copy link
Member Author

But the renderer 'squares' the image, or are we worried about pixalation?

Do you mean the logo shortcode squares the version in the assets directory? that's a bug if it is doing that now since it doesn't previously and that changes behavior.

OK, should we think about clean up?
Removing these square images over time, might remove again several 100MB from the repository.

Absolutely. Given tbat I have moved the necessary ones to the assets/*.png version in this PR, in this PR I could also just delete every file named logo-square.jpg in static/...

@mattstratton
Copy link
Member Author

Absolutely. Given tbat I have moved the necessary ones to the assets/*.png version in this PR, in this PR I could also just delete every file named logo-square.jpg in static/...

okay I take it back - it is still possible to do so, but there are 18 pages in content/events/... which are linking directly to a logo-square.jpg file in the markdown via html.

I would prefer to remove these images from the static directory in a separate PR after this one is merged, and also after #14637 is fixed, so we can use that shortcode in those pages :)

{{- if (where (readDir ($.Scratch.Get "assetsdir")) "Name" "logo.png") -}}
{{- $imagelocation := (printf "events/%s/logo.png" $e.name) -}}
{{- $imageresource := resources.Get $imagelocation -}}
{{- $imagefile := $imageresource.Fit "600x600 webp Lanczos q100" -}}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh. i see what @toshywoshy is talking about.

This shouldn't be the only option, because there are many logos which are NOT square and this is breaking change.

I would say that for the event_logo shortcode, it should not use Image.Fit but instead Image.Resize and we just specify a max width (to keep images from being giant)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care about width?
I would be inclined to say we limit the height, so the text remains on a single screen when you're on a large screen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's arbitrary for this one. have to keep in mind that this shortcode gives a lot of latitude for how it is used so being too prescriptive is going to cause weird and unexpected behavior.

This shortcode tends to be used on the welcome page, but in theory can be used anywere in any part of a page for an event, so we have to be kinda of loose. I also think that #14637 will provide more flexibilty when people want to displaly an image with more control so for now I would leave this shortcode to restrict at least one dimension (to keep it from being too crazy) and then when we bring in the other shortcode, can make this one more prescriptive again

but we can try to limit it via height instead /shrug

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting by height has the advantage of making them all uniform on the line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerdog we're talking about the shortcode used on the individual event welcome pages, so there's only one logo anyway :)

@toshywoshy
Copy link
Contributor

But the renderer 'squares' the image, or are we worried about pixalation?

Do you mean the logo shortcode squares the version in the assets directory? that's a bug if it is doing that now since it doesn't previously and that changes behavior.

Well, yes, I see that as a feature, so I introduced this by adding the 600x600 and using Fit.

OK, should we think about clean up?
Removing these square images over time, might remove again several 100MB from the repository.

Absolutely. Given tbat I have moved the necessary ones to the assets/*.png version in this PR, in this PR I could also just delete every file named logo-square.jpg in static/...

okay I take it back - it is still possible to do so, but there are 18 pages in content/events/... which are linking directly to a logo-square.jpg file in the markdown via html.

I would prefer to remove these images from the static directory in a separate PR after this one is merged, and also after #14637 is fixed, so we can use that shortcode in those pages :)

OK, yes, lets not burden this PR and evaluate the exact consequences, I was more asking about if we should to that, not that we do it now directly

@mattstratton
Copy link
Member Author

Well, yes, I see that as a feature, so I introduced this by adding the 600x600 and using Fit.

I agree that setting a size is a feature but it does change behaviour on a LOT of pages, which is why I made the change in this PR to have it only resize to the width

@mattstratton
Copy link
Member Author

OK, yes, lets not burden this PR and evaluate the exact consequences, I was more asking about if we should to that, not that we do it now directly

understood - i was thinking out loud myself when i suggested that :)

I'll mark this ready for review now then!

@mattstratton mattstratton marked this pull request as ready for review October 1, 2024 12:25
@mattstratton mattstratton requested a review from a team as a code owner October 1, 2024 12:25
@mattstratton mattstratton requested review from a team as code owners October 1, 2024 12:25
Copy link
Contributor

@jerdog jerdog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and helps provide some standardization for images

@mattstratton mattstratton merged commit d5ee680 into main Oct 3, 2024
6 of 7 checks passed
@mattstratton mattstratton deleted the mattstratton/make-em-square-yall branch October 3, 2024 01:23
don-code added a commit to don-code/devopsdays-web that referenced this pull request Oct 16, 2024
Restore functionality to our `event_logo` tag, which was broken in
 devopsdays#14636.

Note that I discovered this as a result of looking into an issue with
 devopsdays#14678, so there may be more affected logos.
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.

logo-square images are increasingly not squares
3 participants