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

Fix item-info color #99761

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 26, 2022

Part of #98341 (the final fix is #93896).

Since the links are already underlined, no need to change their color too. Instead it now uses the text color directly.

Here are some screenshots of the result:

Screenshot from 2022-07-26 15-16-28
Screenshot from 2022-07-26 15-16-33
Screenshot from 2022-07-26 15-16-39

Online demo available here.

cc @jplatte @jsha
r? @notriddle

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 26, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2022
@GuillaumeGomez GuillaumeGomez added the A-rustdoc-ui Area: Rustdoc UI (generated HTML) label Jul 26, 2022
@notriddle
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 26, 2022

📌 Commit 262a48d86c22e36d76892c5a4dce53f8ca00cc73 has been approved by notriddle

It is now in the queue for this repository.

@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 Jul 26, 2022
@jsha
Copy link
Contributor

jsha commented Jul 26, 2022

@bors r-

This is the wrong solution for #98341. The link color means something: you can click here. We should avoid creating exceptions if at all possible. The background for item-infos does not mean anything special. We can choose it to be whatever we want.

The problem here is that the link color in dark theme is chosen to contrast against the dark background, so it's light. But the background for the item-info is also light - too light, in fact. See #93896. In that PR we never landed on the exact color, but that's the direction I think we should go. The text in the item info should be the same color as the body text, and the background should be just different enough from the body background to highlight it, without making it too bright.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 26, 2022
@GuillaumeGomez
Copy link
Member Author

I completely forgot about it and I also prefer your approach. Closing this one then!

@GuillaumeGomez GuillaumeGomez deleted the item-info-color branch July 26, 2022 15:41
@GuillaumeGomez GuillaumeGomez restored the item-info-color branch July 26, 2022 18:52
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jul 26, 2022

@jsha I just realized that this fix is still valid, even though incomplete. It makes the <a> elements' color inherit their parent one. The one you set in #93896. I'm reopening this PR and removing the "fixes" part in the first comment as it is incomplete but still an improvement and will be needed for your PR as well.

@jsha
Copy link
Contributor

jsha commented Jul 27, 2022

It makes the elements' color inherit their parent one.

That's usually not what we want. For instance: a paragraph of text might have color: black. If there's a link in that paragraph we want it to have a distinct color, not black. So it shouldn't inherit it's parent's color.

@GuillaumeGomez
Copy link
Member Author

Ok. I'll wait for your PR to be done and then I'll update the links' color for item info elements.

@GuillaumeGomez GuillaumeGomez added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jul 31, 2022
@GuillaumeGomez
Copy link
Member Author

Blocked on #93896 for the time being.

@bors
Copy link
Contributor

bors commented Aug 2, 2022

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@GuillaumeGomez
Copy link
Member Author

I uploaded the result here.

@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Sep 24, 2022
@GuillaumeGomez
Copy link
Member Author

cc @jsha (sorry, forgot to ping you)

@anden3 anden3 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 22, 2023
@anden3
Copy link
Contributor

anden3 commented Apr 22, 2023

Hello @GuillaumeGomez! There's a merge conflict for this PR, has there been any updates? :)

@GuillaumeGomez
Copy link
Member Author

Still waiting for review.

@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2023
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-review Status: Awaiting review from the assignee but also interested parties. 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.

7 participants