Reintroduce spotlight/"important traits" feature #74111
Conversation
|
Some changes occurred in HTML/CSS/JS. Some changes occurred in HTML/CSS themes. |
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @jyn514 |
|
Sorry but I'm still not in favor of this feature and the @rust-lang/rustdoc already voted to opt it out. It has been proven to be more noise than useful (and not only from the twitter thread linked in the issue). |
|
I feel like closing this is premature. I don't see an FCP for removal anywhere, and based on @kinnison's comments on #73785 there's not really consensus amongst T-rustdoc that this isn't a good feature to have, rather, it was deemed "not horrible" to remove given your (strong?) opinion, especially because reverting that decision was relatively easy. I for one continue to miss this feature pretty much every time I'm working with some non-obvious iterator where knowing the item type is super useful. The same goes for interactions with futures libraries, where it's particularly painful to figure out the item type. If the problem is the choice of trait to display, IMO, we can require that the trait must be annotated with something like I don't think I've seen any discussion of why you're against it beyond it being too much information -- but the key point to me is that this is just enough information. I don't usually care about anything except for the Item/Output associated type, I already have a pretty good idea about the methods etc that Iterator and/or FutureExt or so provide. |
|
Like I said, this decision doesn't come out of nowhere. It had a few downsides but the main one was simply that users didn't use it (and I don't have this information only from the twitter thread like I said). We kept this feature around for a while and whenever I talked about it, people just realized it existed and didn't find it useful in most cases and often they just didn't understand what it was used for. The other big issue was the amount of JS code needed for it. Maintaining something used by a very small minority of users is not worth it considering how much time it takes me to maintain them. Also, "important traits" or whatever the name still remains a very anecdotal information. Anyway, like I said, the rustdoc team opted it. If you want to debate about this feature, we can do it in an issue so no one loses more time than necessary writing code to get it closed like this (I really feel bad about that because of the time @Manishearth took to write this PR...). But as is, this is a no-go. |
|
So after discussion with @Manishearth, we came to this conclusion: a few more steps will be required to reach a good enough level for everyone to be able to enjoy this feature, and in order to do so, we'll do it in multiple steps (asking for feedbacks along the road). For the current PR, I propose the following changes:
|
Manishearth
changed the title
|
@GuillaumeGomez made all the changes. The JS now used by this feature is minimal. |
This makes the spotlight show on hover instead of click. Clicks can be used to persist it, which is also what's used on mobile.
|
☔ The latest upstream changes (presumably #74117) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -2621,6 +2624,16 @@ function defocusSearchBar() { | |||
| }); | |||
| }()); | |||
|
|
|||
| function showImportantTraits(content) { | |||
| let list = content.classList | |||
| @@ -2621,6 +2624,16 @@ function defocusSearchBar() { | |||
| }); | |||
| }()); | |||
|
|
|||
| function showImportantTraits(content) { | |||
| let list = content.classList | |||
| @@ -2621,6 +2624,16 @@ function defocusSearchBar() { | |||
| }); | |||
| }()); | |||
|
|
|||
| function showImportantTraits(content) { | |||
There was a problem hiding this comment.
Actually, this function is never used. Please remove it.
|
|
||
| onEachLazy(document.getElementsByClassName("important-traits"), function(e) { | ||
| e.onclick = function() { | ||
| e.getElementsByClassName('important-traits-tooltiptext')[0].classList.toggle("force-tooltip") |
There was a problem hiding this comment.
I think it's better to use either this or event.target in here (event would be the first argument given to onclick).
There was a problem hiding this comment.
Wouldn't that just be e anyway?
There was a problem hiding this comment.
I'm not too confident about the variable capture process in JS so I prefer to minimize it as much as possible.
|
|
||
| onEachLazy(document.getElementsByClassName("important-traits"), function(e) { | ||
| e.onclick = function() { | ||
| e.getElementsByClassName('important-traits-tooltiptext')[0].classList.toggle("force-tooltip") |
|
You'll need to add the CSS changes inside the newly added |
| @@ -363,6 +363,7 @@ function defocusSearchBar() { | |||
| function handleEscape(ev) { | |||
| var help = getHelpElement(); | |||
| var search = getSearchElement(); | |||
| hideModal(); | |||
There was a problem hiding this comment.
Why did you add the hideModal calls BTW?
There was a problem hiding this comment.
Didn't intend to. Will fix.
| @@ -337,6 +337,11 @@ pre.ignore:hover, .information:hover + pre.ignore { | |||
| border-color: transparent black transparent transparent; | |||
| } | |||
|
|
|||
| .important-traits-tooltiptext { | |||
| background-color: #111 !important; | |||
There was a problem hiding this comment.
Do you really need !important? Generally, just giving a more precise selector allows to override things without needing them.
There was a problem hiding this comment.
Yes, because we're just overriding the same declaration in the themeless version. I added a decl in the themeless version with the intent of having okayish defaults.
There was a problem hiding this comment.
Maybe remove it from the themeless version then? We always have themes after all...
|
Everything addressed except for the Ayu thing. I'll add that shortly after trying out my rebased build |
|
Hmm, i've updated my branch but the PR seems frozen https://github.com/Manishearth/rust/tree/re-spotlight |
|
Reopened as #74370 |
…meGomez Reintroduce spotlight / "important traits" feature (Reopened version of rust-lang#74111 because Github is broken, see discussion there) Fixes rust-lang#73785 This PR reintroduces the "spotlight" ("important traits") feature. A couple changes have been made: As there were concerns about its visibility, it has been moved to be next to the return type, as opposed to being on the side. It also no longer produces a modal, it shows the traits on hover, and it can be clicked on to pin the hover bubble.   It also works fine on mobile: 
…meGomez Reintroduce spotlight / "important traits" feature (Reopened version of rust-lang#74111 because Github is broken, see discussion there) Fixes rust-lang#73785 This PR reintroduces the "spotlight" ("important traits") feature. A couple changes have been made: As there were concerns about its visibility, it has been moved to be next to the return type, as opposed to being on the side. It also no longer produces a modal, it shows the traits on hover, and it can be clicked on to pin the hover bubble.   It also works fine on mobile: 
Fixes #73785
This PR reintroduces the "spotlight" ("important traits") feature.
A couple changes have been made:
As there were concerns about its visibility, it has been moved to be next to the return type, as opposed to being on the side.
It also no longer produces a modal, it shows the traits on hover, and it can be clicked on to pin the hover bubble.
It also works fine on mobile: