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

Reintroduce spotlight feature ("important traits") #73785

Closed
Manishearth opened this issue Jun 27, 2020 · 9 comments · Fixed by #74370
Closed

Reintroduce spotlight feature ("important traits") #73785

Manishearth opened this issue Jun 27, 2020 · 9 comments · Fixed by #74370
Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

I recently noticed that the "Important traits" feature (called "spotlight") was removed in #69514

The stated justification was a twitter poll, however from going through the responses there it seemed like people hadn't noticed it, not that people didn't find it useful. I don't think removing a feature is the right call if people aren't noticing it, that's a UX concern. It's also not that much code.

Can we reintroduce this UI as a little bubble on the return type? Putting it in the sidebar as a tiny circle was guaranteed to make it disappear. I found this feature quite useful, especially for iterators. In particular, my experience resonates with @Mark-Simulacrum's: #69514 (comment) you need to click, scroll to the iterator impl, and then compare pages and substitute return types, whereas here it's all in one place.

I think we have a good indication that we should experiment with the UI and find something that works.

Ultimately, even if a feature is useful for some and not most, we shouldn't be removing it unless it's a net negative impact on the "most", which is definitely not clear here.

(I kinda wish that removing this feature had been FCPd)

cc @GuillaumeGomez @kinnison @rust-lang/rustdoc

@Manishearth Manishearth added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jun 27, 2020
@Manishearth
Copy link
Member Author

As far as the concern about the name "important traits" being confusing, we can perhaps call it "notable traits" or something

@hawkw
Copy link
Contributor

hawkw commented Jun 27, 2020

For what it's worth, I think removing a potentially useful feature based on an informal Twitter poll where the posed question was not about removing it was a mistake. The questions "do you use $FEATURE?" and "should $FEATURE be removed?" are pretty disjunct: I would probably click "no" on a "do you use..." question for a solid 2/3rds of std, but I imagine some people probably use file I/O and would be pretty upset if it was removed.

This is especially harmful for features that might improve accessibility for new users — who are almost certainly the primary audience for "important traits".

An alternative interpretation of the Twitter poll cited as motivation for #69514 suggests that perhaps the spotlight icon should have been made more prominent:
image

@kinnison
Copy link
Contributor

As I said on the PR, it ought to be moderately easy to revert if that's the consensus. Guillaume was annoyed at the code needed to support it, combined with people not understanding it, so I think that if we decide to revert (or reinstate in some other way) the feature, it needs to be better presented/documented, I simply don't know how to do that.

@Manishearth
Copy link
Member Author

Honestly it doesn't seem like a lot of code to me.

We can easily better document it imo. That's a super minor barrier.

We should open a revert PR.

@Manishearth
Copy link
Member Author

I feel like as a first step for better presentation, we can stick the little info bubble next to the return type

@Manishearth
Copy link
Member Author

Made a PR reintroducing it: #74111

I still don't see where it's a lot of code.

@GuillaumeGomez
Copy link
Member

The JS was a lot of code mostly. The feature as is is unclear and noisy.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jul 6, 2020

Taking the discussion here (from #74111 (comment)).

I see your point that it was somewhat hard to discover. I don't know why that's not a reason to explore the feature, though -- it seems like an argument against a particular implementation, I guess, but not the feature itself. I for one wouldn't mind exposing the details w/o a modal, e.g., on the next line after the feature. Manish already moved the info button to the return type which IMO makes it at least a good bit more discoverable.

I also don't really see the JS code as being all that sizeable? Manish's PR adds ~30 lines of JS code. That code looks like it wouldn't be needed if we were to always expose the information rather than sticking it in a modal, as well.

Anyway, like I said, the rustdoc team opted it.

I continue to find this concerning, as I do not see any decision about this from the rustdoc team -- the PR removing this feature clearly had support from you, but I don't personally think that can be said for the other members of the rustdoc team. Regardless, IMO, we should try to make such decisions through FCP to give community members time to respond to the team's consensus. It feels like this should be particularly true when their is some contention around the decision, which, in this case there is -- both Manish and I at least have noted that we find the feature very useful.

@Manishearth
Copy link
Member Author

Manishearth commented Jul 7, 2020

I do think moving forward removing a feature should have a visible FCP.

Manishearth added a commit to Manishearth/rust that referenced this issue Jul 16, 2020
…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.

![image](https://user-images.githubusercontent.com/1617736/86674555-a82d2600-bfad-11ea-9a4a-a1a9ffd66ae5.png)

![image](https://user-images.githubusercontent.com/1617736/86674533-a1061800-bfad-11ea-9e8a-c62ad86ed0d7.png)

It also works fine on mobile:

![image](https://user-images.githubusercontent.com/1617736/86674638-bda25000-bfad-11ea-8d8d-1798b608923e.png)
@bors bors closed this as completed in fc09817 Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
5 participants