Skip to content

Explicitly set height on rust logo <img> element in docs #60272

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

Merged
merged 4 commits into from
May 20, 2019

Conversation

jakst
Copy link
Contributor

@jakst jakst commented Apr 25, 2019

The layout of the left side menu in docs reflows when navigating between pages because of missing height on the element of rust logo.

Setting height='100' tells the browser to reserve that vertical space, leading to a less janky experience.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2019
@GuillaumeGomez
Copy link
Member

height and width attribute on the dom element forces the image resize. We don't want that but instead to change it's max-height and max-width.

@jakst
Copy link
Contributor Author

jakst commented Apr 26, 2019

That is already not the case, because the img-element already has width='100', so right now you get the worst of both world where the img is forced to 100x100, without the browser knowing about the height on the first render pass and therefore needing to reflow the sidebar vertically when it has finished loading the image.

@jakst
Copy link
Contributor Author

jakst commented Apr 26, 2019

I realise we are talking about different things. I thought you meant for scaling up, but it is clear to me that we now break the scaling down scenario. Will take another look.

@jakst
Copy link
Contributor Author

jakst commented Apr 27, 2019

I have fixed the scaling down issues. Will you take another look @GuillaumeGomez ?

@GuillaumeGomez
Copy link
Member

I'm not sure it fixes all issues considering you kept the width. If the image has a higher height than width, it'll be an issue as well. :) A bit more requires to be done in here.

@jakst
Copy link
Contributor Author

jakst commented Apr 28, 2019

Good input, although the image does not have a higher height than width at the moment, it's 1:1. I'm thinking it would be reasonable to alter the style if the logo is changed to some other ratio. padding-bottom: 50% only works for an image with a 1:1 ratio anyway, so this is not a general solution.

I can not spend any more time on this unfortunately. In my opinion, loss of adaptiveness to any aspect ratio (which affects nobody at this time) is a reasonable tradeoff to reduce layout reflow (which affects everyone right now). If you do not agree I think we should close this PR, otherwise - aim to merge it as it is now.

@GuillaumeGomez
Copy link
Member

I'll push on your branch then. I have done this in the past on some personal project so I just need to get the CSS fix but you weren't far from it.

@jakst
Copy link
Contributor Author

jakst commented Apr 28, 2019

Feel free to do so. I will be sure to study your solution :)

@GuillaumeGomez
Copy link
Member

Thanks for starting it in any case. I'll try to do it tomorrow.

jakst added 3 commits April 29, 2019 10:38
The layout of the left side menu in docs reflows when navigating between pages because of missing height on the <img> element of rust logo.

Setting height='100' tells the browser to reserve that vertical space, leading to a less janky experience.
This reverts commit d79a01b72f4722611cb21b719e6243aad3e7ec3c.
@GuillaumeGomez
Copy link
Member

And done!

@jakst Does it seem good to you?

@jakst
Copy link
Contributor Author

jakst commented Apr 29, 2019

Works well on desktop, but needs adjustment for @media (max-width: 700px).

@GuillaumeGomez
Copy link
Member

Fixed it for mobile as well. Good catch!

@jakst
Copy link
Contributor Author

jakst commented Apr 29, 2019

Looking good to me now! 👌

@GuillaumeGomez
Copy link
Member

r? @QuietMisdreavus

@Mark-Simulacrum
Copy link
Member

Visiting for triage -- @QuietMisdreavus this looks like it's waiting on a review from you; will you get a chance to make that review happen soon?

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented May 18, 2019

I'll switch reviewers, @QuietMisdreavus is focused on docs.rs for the moment.

r? @Manishearth

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented May 19, 2019

📌 Commit 9db0fd7 has been approved by Manishearth

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2019
@bors
Copy link
Collaborator

bors commented May 19, 2019

⌛ Testing commit 9db0fd7 with merge a5000c5...

bors added a commit that referenced this pull request May 19, 2019
Explicitly set height on rust logo <img> element in docs

The layout of the left side menu in docs reflows when navigating between pages because of missing height on the <img> element of rust logo.

Setting height='100' tells the browser to reserve that vertical space, leading to a less janky experience.
@bors
Copy link
Collaborator

bors commented May 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Manishearth
Pushing a5000c5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 20, 2019
@bors bors merged commit 9db0fd7 into rust-lang:master May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants