-
Notifications
You must be signed in to change notification settings - Fork 4k
Support custom cover heights #3779
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
base: main
Are you sure you want to change the base?
Conversation
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
a207335 to
45e5c86
Compare
45e5c86 to
9cfddb9
Compare
9cfddb9 to
9cd0359
Compare
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.
Isn't it an issue that both light and dark image use the same sizes/aspect-ratio/position ?
|
|
||
| // Load the original image (using src, not srcSet) to get true dimensions | ||
| // Use dark image if available, otherwise fall back to light | ||
| const imageToLoad = imgs.dark || imgs.light; |
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 understand why we take the dark one here. Everywhere else it tries from light image first
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 get what you mean, but it was intentional though. Its to account for if the dark img exists, it means it was intentionally uploaded as the light is the default and dark an explicit opt-in. So when we get into the situation where the img didnt ship with a size payload and we manually need to check the dimensions, we just first check if the dark one exists here to avoid always going for the light one directly and possibly not accounting for a dark mode with differign dimensions. We still fallback to light anyway if there is no dark one.
Yeah it is, I listed the same as a current limitation somewhere in another PR. I chatted about it with samy but while it'd be nice to support heights and object fit on both dark and light mode individually, its just not included yet in this feature release. |
This PR handles the GBO-side of enabling cover height resizing. The basic idea is:
heightexists on the page cover, it means the cover was resized. because we can't/shouldnt enforce an aspect ratio on a cover that has a dynamic height, different logic kicks in to determine the appropriate height depending on the dimensions so that the entire image is contained within the coverThe previous PR was reverted, as we didn't do step 1 above: we applied the 'new' sizing rules to existing page covers, which causes a lot of existing page covers to regress.
To help explain how a page cover has a fixed aspect ratio by default but how setting a height will impact that, we have tooltip UI in the editor + I have a CR ready to add more details in the support article.
CleanShot.2025-11-03.at.15.21.03.mp4