-
Notifications
You must be signed in to change notification settings - Fork 373
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
CLDR-17947 site: pin image width, win fix #4054
Conversation
✨ deployed to https://b1085791.cldr.pages.dev |
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.
Small enough change and I know the context so I'm happy to accept.
For future PRs -- when you post PRs can you include screenshots and links to the before/after? It will help future CLDR developers have context for the fix.
From slack here's an example page that is helped by this fix: https://cldr.unicode.org/index/language-support-levels
@@ -180,3 +180,8 @@ header#atViewHeader { | |||
div > header { | |||
display: none !important; | |||
} | |||
|
|||
img { | |||
max-width: 100%; |
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.
Sounds like we should use svh & svw
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.
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.
@macchiati what are you seeing instead? When I used dvh/dvw
from your original proposal, the scale didn't work properly- it was far too wide still.
@@ -180,3 +180,8 @@ header#atViewHeader { | |||
div > header { | |||
display: none !important; | |||
} | |||
|
|||
img { | |||
max-width: 100%; |
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.
… On Mon, Sep 16, 2024 at 12:21 PM Conrad Nied ***@***.***> wrote:
***@***.**** approved this pull request.
Small enough change and I know the context so I'm happy to accept.
For future PRs -- when you post PRs can you include screenshots and links
to the before/after? It will help future CLDR developers have context for
the fix.
From slack here's an example page that is helped by this fix:
https://cldr.unicode.org/index/language-support-levels
—
Reply to this email directly, view it on GitHub
<#4054 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACJLEMCZ37ZQNKLHJNL7RELZW4VUDAVCNFSM6AAAAABOJ5NEE6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMBXGU4DGNRRGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
added to the top |
img {
max-width: 100svw;
max-height: 90svh;
} gives this which looks wrong to me (still a huge image, twice as wide as the page): compare to the current PR version: https://b1085791.cldr.pages.dev/index/language-support-levels#language-selection-ui img {
max-width: 100%;
max-height: 90dvh;
} Do you prefer the former? (the '90' is so that a super tall image still leaves some text above or below the image, so you can see there's something to scroll to.) |
That is very strange. Let's go with what you have; we can check out alternatives later. |
Just tried it out; looks great. |
CLDR-17947
also:
fix builder for windows pathnames
This PR completes the ticket.
Testing
ALLOW_MANY_COMMITS=true