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.

Sign up for GitHub

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

Jump to bottom

Apply rust-logo class only on default logo #91958

Merged
merged 1 commit into from
Dec 16, 2021
Merged

Apply rust-logo class only on default logo #91958

merged 1 commit into from
Dec 16, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 15, 2021

@jsha jsha 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 Dec 15, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 15, 2021
@jsha jsha mentioned this pull request Dec 15, 2021
Closed
@jhpratt
Copy link
Member

jhpratt commented Dec 15, 2021

I feel like having an environment variable would make most sense. Either opt-in or opt-out; I'd personally prefer the latter. That way we don't keep switching back and forth.

@jsha
Copy link
Contributor Author

jsha commented Dec 15, 2021

I fixed the missing alt. I also took the opportunity to replace some inconsistent ' with " (which we use everywhere else).

@jhpratt said:

I feel like having an environment variable would make most sense. Either opt-in or opt-out; I'd personally prefer the latter. That way we don't keep switching back and forth.

I don't understand this comment.

@GuillaumeGomez
Copy link
Member

With my second commit, I realized that we didn't have a test to ensure that the image in the sub-logo was the right one. Can you add one or open an issue please? If you prefer to not do it in this PR, r=me (but still open the issue ;) ).

@jsha
Apply rust-logo class only on default logo
246de45 
Also replace ' with " in rustdoc template
@GuillaumeGomez
Copy link
Member

Awesome, thanks! r=me once CI pass

@jhpratt
Copy link
Member

jhpratt commented Dec 15, 2021

I don't understand this comment.

If we used an environment variable to control whether the outline is present, then it would be fully under control of the crate. If I remember correctly, whether the outline applies to every logo has gone back and forth a few times. Personally I would like to have it for my crates, but others (such as you) don't. So why not make it configurable? 🤷‍♂️

@GuillaumeGomez
Copy link
Member

You can instead use --html-in-header and add the style you want.

@jhpratt
Copy link
Member

jhpratt commented Dec 15, 2021

Certainly. But for common things, which I imagine this is, configuration makes sense imo.

@jsha
Copy link
Contributor Author

jsha commented Dec 15, 2021

whether the outline applies to every logo has gone back and forth a few times

I don't know the full history here, but at least the most recent change here was an unintentional bug, not a design choice, which is what I'm trying to fix. I'm not aware of a time when outlining was intentionally applied to non-default logos.

@GuillaumeGomez
Copy link
Member

@jhpratt I don't think having such things handled by environment variables will be any better. Also, it was overwhelmingly in favor of not changing the display of the logos provided by users. Use the existing options.

@jsha I can't find the link but in short it was something I think to have a "border" applied on all logos to make them look "good" whatever the theme.

@jsha
Copy link
Contributor Author

jsha commented Dec 16, 2021

@bors r=GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Dec 16, 2021

📌 Commit 246de45 has been approved by GuillaumeGomez

@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 Dec 16, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 16, 2021
@matthiaskrgr
Rollup merge of rust-lang#91958 - jsha:rust-logo, r=GuillaumeGomez
35c5247 
Apply rust-logo class only on default logo

Fixes rust-lang#91653.

![image](https://user-images.githubusercontent.com/220205/146138145-a7a62ea6-3205-4bc7-8460-e985284d93ea.png)

Demo: https://rustdoc.crud.net/jsha/hashes/sha2/

r? `@GuillaumeGomez`
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 16, 2021
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 16, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2021
@bors
Auto merge of rust-lang#91996 - matthiaskrgr:rollup-8pdt8x7, r=matthi…
f840216 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#91901 (Remove `in_band_lifetimes` from `rustc_symbol_mangling`)
 - rust-lang#91904 (Remove `in_band_lifetimes` from `rustc_trait_selection`)
 - rust-lang#91951 (update stdarch)
 - rust-lang#91958 (Apply rust-logo class only on default logo)
 - rust-lang#91972 (link to pref_align_of tracking issue)
 - rust-lang#91986 (Bump compiler-builtins to 0.1.66)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e1d1a28 into rust-lang:master Dec 16, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

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
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.

Adaptive SVG logos look terrible on dark themes
6 participants
@jsha @jhpratt @GuillaumeGomez @bors @rust-highfive @rustbot