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

fix(shs-5179): adding missing style settings #1430

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

mndonx
Copy link
Contributor

@mndonx mndonx commented Jan 6, 2024

READY FOR REVIEW

Summary

This PR fixes an issue where the vertical card linked image style is missing any settings at mobile sizes.

Urgency

medium

Steps to Test

  1. Visit a page like: https://southasia.suhumsci.loc/
  2. Note that there is now a preset for the smallest width here where there was not before:
    https://southasia.suhumsci.loc/admin/config/media/image-styles/manage/postcard_vertical_linked_465px_

Before:
Screenshot from 2024-01-06 13-55-04

Because of the nature of local testing, I'm having trouble ensuring that it is creating new images properly. I flushed the images, but I don't know if it can create new images locally? Right now, for me, they are showing up broken.

PR Checklist

@mndonx mndonx requested a review from cienvaras January 6, 2024 22:43
@mndonx
Copy link
Contributor Author

mndonx commented Jan 6, 2024

@cienvaras This first fix is ready for review. There was no configuration associated with the smallest Postcard Vertical Linked image style, so it was displaying the original image at small widths, I believe. Or maybe it shows the fallback image style image -- either way, this fix will probably improve load times quite a bit at mobile for many sites.

The other issue I found with image styles involves departments overriding the default Publication views, choosing the incorrect image style for the publication images.

Here's the default using the "Publications Grid" responsive image style:
Screenshot from 2024-01-06 14-38-20

And here's the English site's "homepage bookshelf" configuration
Screenshot from 2024-01-06 15-19-01

I believe this listing (and others, perhaps) should use the same responsive image style as the "publications grid" and that fix will save some pixels.

However, I don't believe that's in our domain to fix, since it is a views override. Am I correct about that? Once I hear back about that, I'll write up something a little more detailed for the Clickup ticket.

Thank you!
Amanda

@cienvaras cienvaras changed the base branch from develop to fk-stnfd-sprint-41 January 12, 2024 16:56
@cienvaras
Copy link
Collaborator

@mndonx First of all, sorry for the delay in reviewing this PR, there was a lot in the queue this week 😅

The image style update works as expected, I'm merging this PR in the release branch. And you're right, fixing the view(s) is not in our hands, the H&S team should do those changes manually. Please add your findings to the ClickUp ticket.

@cienvaras cienvaras merged commit 4d243ee into fk-stnfd-sprint-41 Jan 12, 2024
4 of 8 checks passed
@cienvaras cienvaras deleted the SHS-5179-optimize-mobile-images branch January 12, 2024 16:59
@cienvaras
Copy link
Collaborator

@mndonx btw, for testing this locally, the best way to go is to upload a new image that doesn't exists in the live site. In local stage file proxy is enabled, so it will pull all existing images from the live site. That's why flushing doesn't work.

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

Successfully merging this pull request may close these issues.

2 participants