fix(event-landing): enforce 16:9 aspect ratio on banner - #834
Conversation
Replace the fixed max-height with an aspect-ratio so the banner scales responsively, and center the image with object-position. Signed-off-by: Reza Jelveh <fishmangit@dynamia.ai>
hami-robot
Bot
added
the
kind/cleanup
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fishman The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for project-hami ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| display: block; | ||
| width: 100%; | ||
| max-height: 480px; | ||
| aspect-ratio: 16 / 9; |
There was a problem hiding this comment.
this drops the 480px cap, so on a 1140px container the banner is now about 640px tall. in #746 the ask was that banners already look too large on desktop. does this make that worse?
There was a problem hiding this comment.
The bigger issue is the Kubecon China banner, but that's a normal format. Almost all conference banners are 16/9
| display: block; | ||
| width: 100%; | ||
| max-height: 480px; | ||
| aspect-ratio: 16 / 9; |
There was a problem hiding this comment.
the kcd vietnam banner is 1920x1920 and the kubecon jp one is 1920x1080. so cover cuts 44 percent off the square one, which is the banner #746 is about, and leaves the other untouched. is contain plus a background closer to what you want?
There was a problem hiding this comment.
Everything cuts a part off, the Vietnam banner is 4:3 but allows for 16:9 to keep all the important content.
There was a problem hiding this comment.
i measured the file on master and it is 1920x1920, so 1:1 rather than 4:3. at 16:9 that drops 44 percent of the height instead of 25. still fine for the important content?
| max-height: 480px; | ||
| aspect-ratio: 16 / 9; | ||
| object-fit: cover; | ||
| object-position: center; |
There was a problem hiding this comment.
object-position already defaults to center, so this line changes nothing. keep it as documentation, or drop it?
There was a problem hiding this comment.
Not sure, will double check but if true, probably drop it. The other pr that set a fixed width seemed to have caused it to be left aligned, but maybe I saw it wrong.
2 participants
Replace the fixed max-height with an aspect-ratio so the banner scales
responsively, and center the image with object-position.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Some banners have an odd aspect-ratio. We should enforce 16:9 as default.
Also requires the same aspect-ratio from the KubeCon China banner. @rootsongjc
Which issue(s) this PR fixes:
Fixes #746
Also supercedes #752
Checklist:
npm run lintandnpm run format:checkpassnpm run buildsucceeds for bothenandzhgit commit -s)