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

Add methods properties in emoji to sidebar #67871

Closed
wants to merge 2 commits into from

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Jan 4, 2020

Fix #67869

2020-01-05-151241_179x783_scrot

emojis += if is_rustc_private { "⚙️" } else { "🔬" };
};
if item.deprecation().is_some() {
emojis += "⚰️";
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this better

Suggested change
emojis += "⚰️";
emojis += "🔥";

Copy link
Contributor

@euclio euclio Jan 4, 2020

Choose a reason for hiding this comment

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

Other options to consider:

  • 👎
  • 🕸
  • 🚨
  • 🚧
  • 🛑
  • 🚫
  • 🟡
  • 🟨
  • 🚩
  • ⚠️

I'm personally partial to 🚧 (followed by 🚩). think they convey the message of "hey, pay attention, this will go away at some point" without being too strong.

EDIT: 🚧 may have too much of a "under construction, stabilizing" connotation than we would like, though.

Copy link
Contributor

@euclio euclio Jan 4, 2020

Choose a reason for hiding this comment

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

Also, notably, MDN uses 👎 for deprecated APIs.

Copy link
Contributor Author

@pickfire pickfire Jan 5, 2020

Choose a reason for hiding this comment

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

Yeah, we should bikeshed which emoji to use, right now there is not one to use. The one I suggested is a coffin. I think I prefer 👎. Can there even be a vote or something for this?

@pickfire
Copy link
Contributor Author

pickfire commented Jan 5, 2020

Added the screenshots.

src/librustdoc/html/render.rs Outdated Show resolved Hide resolved
src/librustdoc/html/render.rs Show resolved Hide resolved
@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2020
@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-01-05T14:33:12.5450769Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-05T14:33:12.5696389Z ##[command]git config gc.auto 0
2020-01-05T14:33:12.5766109Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-05T14:33:12.5820095Z ##[command]git config --get-all http.proxy
2020-01-05T14:33:12.5959879Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67871/merge:refs/remotes/pull/67871/merge
---
2020-01-05T14:42:17.3432025Z     Checking rustc-rayon v0.3.0
2020-01-05T14:42:26.6566102Z    Compiling serde_derive v1.0.81
2020-01-05T14:42:49.6886125Z     Checking serde_json v1.0.40
2020-01-05T14:42:50.8992511Z     Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
2020-01-05T14:42:53.0866262Z error[E0599]: no method named `as_deref` found for type `std::option::Option<clean::types::Stability>` in the current scope
2020-01-05T14:42:53.0867108Z     --> src/librustdoc/html/render.rs:4072:56
2020-01-05T14:42:53.0867607Z      |
2020-01-05T14:42:53.0868154Z 4072 |                     if let Some(stab) = item.stability.as_deref().filter(|stab| stab.level == stability::Unstable) {
2020-01-05T14:42:53.0869421Z      |
2020-01-05T14:42:53.0869902Z      = note: the method `as_deref` exists but the following trait bounds were not satisfied:
2020-01-05T14:42:53.0869902Z      = note: the method `as_deref` exists but the following trait bounds were not satisfied:
2020-01-05T14:42:53.0870350Z              `clean::types::Stability : std::ops::Deref`
2020-01-05T14:42:53.4533492Z error: aborting due to previous error
2020-01-05T14:42:53.4534171Z 
2020-01-05T14:42:53.4534656Z For more information about this error, try `rustc --explain E0599`.
2020-01-05T14:42:53.4657437Z error: could not compile `rustdoc`.
---
2020-01-05T14:42:53.4765738Z   local time: Sun Jan  5 14:42:53 UTC 2020
2020-01-05T14:42:53.7422412Z   network time: Sun, 05 Jan 2020 14:42:53 GMT
2020-01-05T14:42:53.7425282Z == end clock drift check ==
2020-01-05T14:42:55.1983338Z 
2020-01-05T14:42:55.2085642Z ##[error]Bash exited with code '1'.
2020-01-05T14:42:55.2118796Z ##[section]Starting: Checkout
2020-01-05T14:42:55.2120603Z ==============================================================================
2020-01-05T14:42:55.2120657Z Task         : Get sources
2020-01-05T14:42:55.2120701Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@GuillaumeGomez
Copy link
Member

I'm not a big fan of the rendering. The icons are too small... Also, this is quite the UI change so cc @rust-lang/rustdoc

@kinnison
Copy link
Contributor

kinnison commented Jan 7, 2020

While I don't object to this on principle, I would prefer to see something which helps those who don't grok images. Either tooltips, or else some help somewhere to explain the iconography.

@GuillaumeGomez
Copy link
Member

That's my issue with icons: we have to explain them. :-/

@kinnison
Copy link
Contributor

kinnison commented Jan 8, 2020

I don't object to them being there so long as they're explained. tooltips would be most intuitive for people. Once they learn, the emoji will be very good from an 'at a glance' ability to recognise deprecated names etc.

@GuillaumeGomez
Copy link
Member

Anyway, for unsafe we already have one which looks like this: /!\ (yep, now imagine it "in picture" haha)

@pickfire
Copy link
Contributor Author

pickfire commented Jan 10, 2020

What if we were to do the same to reduce the opacity of those that is unstable and add a strikethrough for those that are deprecated, like on the other methods in addition to this?

@kinnison
Copy link
Contributor

I would be concerned that reduced opacity or strikethrough would reduce readability in the sidebar which is, to some extent, an accessibility issue. At least added emoji don't reduce the readability of the text.

@pickfire
Copy link
Contributor Author

pickfire commented Jan 10, 2020

@kinnison That is also one of my concern and why I prefer emoji over that. I also tried using a big one instead of subscript, it looks very distracting that way.

However, I am way more concerned with using different type of hints in different places, such as using this in sidebar, both text style and this emoji superscript in page bottom references, using weird box with some emoji and some text for the main text. Looks kinda congruent but still weird, and also the search does not include any of these, but it may be further improved.

@GuillaumeGomez
Copy link
Member

So here what I suggest @pickfire: I'm not a big fan of emojis but it could improve readability if done correctly. So let's open an issue where we can discuss it. When we'll reach a consensus, we can come back to this PR so you end the implementation. Sounds ok to you? (and to everyone else?)

@pickfire
Copy link
Contributor Author

Yup, looks good to me. So we keep this PR open and just reuse it for later or close it and open one again if needed?

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jan 13, 2020

As you prefer. Keeping this one open seems fine to me but other rust people might disagree. So I personally think it's ok. :)

@Dylan-DPC-zz
Copy link

Closing it then :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sidebar emoji to indicate properties for methods in rustdoc
8 participants