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

Use adaptive SVG favicon for rustdoc like other rust sites #75438

Merged
merged 1 commit into from
Sep 25, 2020
Merged

Use adaptive SVG favicon for rustdoc like other rust sites #75438

merged 1 commit into from
Sep 25, 2020

Conversation

Cldfire
Copy link
Contributor

@Cldfire Cldfire commented Aug 12, 2020 •
edited
Loading

Use the theme-adaptive SVG favicon that was recently introduced for the Rust site (and others).

(This PR is simply copied from the PR linked above, so see that for rationale.)

Closes #72165.

Before, Firefox on Linux:

image

After, Firefox on Linux (prefers-color-scheme set to dark by setting ui.systemUsesDarkTheme to a number value of 1 in about:config):

image

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2020
@Cldfire
Copy link
Contributor Author

Cldfire commented Aug 12, 2020

r? @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

I am in a dark theme and yet the favicon are all visible. I'm not sure how reliable this is overall...

@Cldfire
Copy link
Contributor Author

Cldfire commented Aug 12, 2020

I am in a dark theme and yet the favicon are all visible. I'm not sure how reliable this is overall...

Yeah, prefers-color-scheme isn't super widespread yet. This is still an improvement for many people over the way it was before (no attempt to handle dark themes), though.

We could just stick a white outline around the favicon like what we did for the logo, but I think there's some value in doing things the same way other Rust web properties have chosen to do it.

@GuillaumeGomez
Copy link
Member

We could just stick a white outline around the favicon like what we did for the logo, but I think there's some value in doing things the same way other Rust web properties have chosen to do it.

I'd prefer that if you don't mind. Let's try to keep only one strategy overall.

@ollie27 ollie27 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 12, 2020
@jyn514 jyn514 added the A-rustdoc-themes Area: Themes for HTML pages generated by rustdoc label Aug 25, 2020
@Cldfire
Copy link
Contributor Author

Cldfire commented Aug 26, 2020 •
edited
Loading

Well, I tried the white outline approach:

image

But it just doesn't look good for the favicon:

08-25-20_21:28

There aren't enough pixels to work with for the outline to make sense.

I also tried just using the fallback PNG favicons from the PR:

image

But I really don't like the look of that either. It just feels... wrong.

I would like to stick with the SVG favicon because:

  1. It looks great
  2. It is consistent with the other Rust websites, so if the user gets the right favicon color everywhere else they'll get it in Rustdoc too

Note that mdbook also uses this approach.

@Cldfire
Copy link
Contributor Author

Cldfire commented Aug 26, 2020

Also note that this is the same approach GitHub uses for their favicon. ui.systemUsesDarkTheme set to 0:

image

Set to 1:

image

I've also seen this approach widely recommended online. It may not be perfectly supported now but it seems to be the future.

@crlf0710
Copy link
Member

crlf0710 commented Sep 11, 2020 •
edited
Loading

Ping from triage, what's the current status on this? Also i saw there's a merge commit in this PR. @Cldfire needs to rebase the commits before it can be accepted.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2020
@Dylan-DPC-zz Dylan-DPC-zz 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 Sep 11, 2020
@Cldfire Cldfire force-pushed the rustdoc/use-adaptive-svg-favicon branch from 8403179 to 3943916 Compare September 13, 2020 22:38
@Cldfire
Copy link
Contributor Author

Cldfire commented Sep 13, 2020

Ping from triage, what's the current status on this?

Waiting on approval / rejection from the rustdoc team.

Also i saw there's a merge commit in this PR, which is unacceptable. @Cldfire needs to rebase the commits before it can be accepted.

Rebased 🙂

@jyn514 jyn514 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 Sep 22, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 22, 2020

ping @GuillaumeGomez (@Manishearth might have opinions too since they were a big part of the decision for #75249)

@GuillaumeGomez
Copy link
Member

Just a nit/question but otherwise looks good to me.

@Manishearth
Copy link
Member

No opinions, as long as this is just for the Rust logo I'm fine with it

@jyn514 jyn514 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 Sep 22, 2020
@Cldfire Cldfire force-pushed the rustdoc/use-adaptive-svg-favicon branch from 3943916 to 085679c Compare September 24, 2020 01:32
@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 24, 2020

📌 Commit 085679c 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 24, 2020
@jonas-schievink jonas-schievink mentioned this pull request Sep 25, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2020
@bors
Auto merge of rust-lang#77172 - jonas-schievink:rollup-a041rou, r=jon…
5bfeee5 
…as-schievink

Rollup of 15 pull requests

Successful merges:

 - rust-lang#75438 (Use adaptive SVG favicon for rustdoc like other rust sites)
 - rust-lang#76304 (Make delegation methods of `std::net::IpAddr` unstably const)
 - rust-lang#76724 (Allow a unique name to be assigned to dataflow graphviz output)
 - rust-lang#76978 (Documented From impls in std/sync/mpsc/mod.rs)
 - rust-lang#77044 (Liballoc bench vec use mem take not replace)
 - rust-lang#77050 (Typo fix: "satsify" -> "satisfy")
 - rust-lang#77074 (add array::from_ref)
 - rust-lang#77078 (Don't use an if guard to check equality with a constant)
 - rust-lang#77079 (Use `Self` in docs when possible)
 - rust-lang#77081 (Merge two almost identical match arms)
 - rust-lang#77121 (Updated html_root_url for compiler crates)
 - rust-lang#77136 (Suggest `const_mut_refs`, not `const_fn` for mutable references in `const fn`)
 - rust-lang#77160 (Suggest `const_fn_transmute`, not `const_fn`)
 - rust-lang#77164 (Remove workaround for deref issue that no longer exists.)
 - rust-lang#77165 (Followup to rust-lang#76673)

Failed merges:

r? `@ghost`
@bors bors merged commit 15efed4 into rust-lang:master Sep 25, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 25, 2020
@Cldfire Cldfire deleted the rustdoc/use-adaptive-svg-favicon branch September 25, 2020 14:28
@sl4m sl4m mentioned this pull request Aug 13, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2021
@bors
Auto merge of rust-lang#87997 - sl4m:update-favicon-order, r=Guillaum…
32c3f2d 
…eGomez

Updates favicon order of precedence as it matters to Chrome

Hi, this updates rust-lang#75438 to fix an order of precedence issue for Chrome. Unfortunately, the last favicon defined wins when it comes to Chrome, hence the primary icon being placed last. I [brought it up](https://bugs.chromium.org/p/chromium/issues/detail?id=1104663) with the Chromium team last year, but so far it's a non-issue.

I've created an example website that mimics the behaviour in Chrome. https://sl4m.github.io/chrome-favicon/

This is what I'm seeing at the moment when viewing https://doc.rust-lang.org/core/index.html in Chrome. It's falling back to the PNG.

<img width="80" alt="Screenshot 2021-08-12 at 21 11 58" src="https://user-images.githubusercontent.com/47347/129304041-b598213e-fcd3-4df1-addb-e6feac6c35b1.png">
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-themes Area: Themes for HTML pages generated by rustdoc 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.48.0
Development

Successfully merging this pull request may close these issues.

Add a white glow or background on the favicon.