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

Unify search input and buttons size #93113

Merged
merged 1 commit into from
Jan 23, 2022
Merged

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 20, 2022

Fixes #93060.

Here what it looks like:

Screenshot from 2022-01-20 21-38-19
Screenshot from 2022-01-20 21-38-22

You can test it here.

r? @jsha

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Jan 20, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 20, 2022
@jsha
Copy link
Contributor

jsha commented Jan 20, 2022

Thanks for working on this! Much improved but it still doesn't look quite right:

ayu:

dark:

light:

Also, doesn't necessarily have to be for this change, but: setting the width of the search bar with calc, and setting the right position of the icons with absolute pixel positions, seems brittle and over-complicated. It would be nice to change that so the icons just use margin to determine their position, and the search bar uses width: auto (the default, I think).

@GuillaumeGomez
Copy link
Member Author

Also, doesn't necessarily have to be for this change, but: setting the width of the search bar with calc, and setting the right position of the icons with absolute pixel positions, seems brittle and over-complicated. It would be nice to change that so the icons just use margin to determine their position, and the search bar uses width: auto (the default, I think).

That can be done in this PR as well, I don't mind.

Thanks for working on this! Much improved but it still doesn't look quite right:

The problem is the height or something else? (The border looks wrong on the dark theme indeed)

@jsha
Copy link
Contributor

jsha commented Jan 20, 2022

The height is maybe 2 pixels too short on top and bottom? And the border-radius looks still looks a little bigger on the buttons than on the search box. And yes, as you said, the border on the search box is wrong in the dark theme.

Note: I think if you pursue the cleanup I mentioned, it might make it easier to fix the height issue. You can set the containing element to the height you want, and make sure the search box and buttons all have height: 100%.

@GuillaumeGomez
Copy link
Member Author

Oh indeed! Will do that then. :)

@GuillaumeGomez GuillaumeGomez force-pushed the unify-sizes branch 2 times, most recently from 5011e79 to 37eeede Compare January 20, 2022 20:52
@jsha
Copy link
Contributor

jsha commented Jan 20, 2022

Thanks! Much improved. For some reason there is still a 1px difference in height (on both top and bottom) between the search-input and the buttons. I can reproduce on both Firefox and Chrome (latest). It's pretty mysterious because the browser tools say both items are exactly 34px tall, but the search input is clearly just a little bit taller (you can see by removing the margin-left from the help button, so they are adjacent to each other).

I can only assume this is some baked-in browser weirdness specific to input fields? Or perhaps I'm missing something.

This is really hacky, but putting this style on search-input fixed in for me locally:

height: calc(100% - 2px);
margin-top: 1px;

On a similar note: .search-input has border-radius: 2px, and so do #help-button and #settings-menu, but if you look at them closely (or screenshot and zoom in), you can see the search-input is slightly more rounded. If you set .search-input's border-radius: 1px, they render the same. Again, I have no idea why this is. 🤷🏻

@bors
Copy link
Contributor

bors commented Jan 21, 2022

☔ The latest upstream changes (presumably #93119) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Interestingly enough, I can't reproduce the height bug on firefox:

Screenshot from 2022-01-21 22-22-50

However I do have it on chrome! That is some crazy dark magic...

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jan 21, 2022

Ok, found it! It was the box-shadow. Removing it since it's not being applied anyway (or at least not correctly)...

@GuillaumeGomez GuillaumeGomez force-pushed the unify-sizes branch 2 times, most recently from b57d430 to c1c232c Compare January 21, 2022 22:26
@GuillaumeGomez
Copy link
Member Author

And done! I updated the uploaded docs as well. It ended up being a nice cleanup. :)

Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent sleuthing! And indeed this wound up being a very nice cleanup.

src/librustdoc/html/static/css/rustdoc.css Show resolved Hide resolved
src/librustdoc/html/static/css/rustdoc.css Show resolved Hide resolved
@jsha
Copy link
Contributor

jsha commented Jan 22, 2022

Looks good! Thanks for doing this cleanup. It looks great.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 22, 2022

📌 Commit f0525da has been approved by jsha

@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 Jan 22, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
This was referenced Jan 22, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#90666 (Stabilize arc_new_cyclic)
 - rust-lang#91122 (impl Not for !)
 - rust-lang#93068 (Fix spacing for `·` between stability and source)
 - rust-lang#93103 (Tweak `expr.await` desugaring `Span`)
 - rust-lang#93113 (Unify search input and buttons size)
 - rust-lang#93168 (update uclibc instructions for new toolchain, add link from platforms doc)
 - rust-lang#93185 (rustdoc: Make some `pub` items crate-private)
 - rust-lang#93196 (Remove dead code from build_helper)

Failed merges:

 - rust-lang#93188 (rustdoc: fix bump down typing search on Safari)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 74b05ce into rust-lang:master Jan 23, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 23, 2022
@GuillaumeGomez GuillaumeGomez deleted the unify-sizes branch January 23, 2022 12:29
@camelid
Copy link
Member

camelid commented Jan 25, 2022

The new search box looks a bit cramped to me. In fact, I half-thought this might have been an accidental regression.

@GuillaumeGomez
Copy link
Member Author

How much bigger (in height I suppose) would you want the search box to be?

@jsha
Copy link
Contributor

jsha commented Jan 25, 2022

Note that one of the issues flagged in #59840 was that the search input was visually bigger than the main heading. I think that makes sense - the main heading should be noticeably bigger than the search box.

@camelid
Copy link
Member

camelid commented Jan 25, 2022

How much bigger (in height I suppose) would you want the search box to be?

It's not really height but padding that's the issue.

@GuillaumeGomez
Copy link
Member Author

I see. How much padding do you think we should use then?

@camelid
Copy link
Member

camelid commented Jan 25, 2022

I don't have a concrete suggestion, other than "a bit more" ;)

@GuillaumeGomez
Copy link
Member Author

Ok, I'll send a PR in the next days and you'll tell me then if it's good for you. 😆

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 16, 2022
…, r=jsha

Add a bit more padding in search box

As asked in rust-lang#93113 (comment), here is a bit more padding.

You can check it [here](https://rustdoc.crud.net/imperio/search-input-padding/foo/index.html).

r? `@camelid`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: fix border-radius and sizing of help and settings icons
6 participants