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

[rustdoc] Add sub settings #60778

Open
wants to merge 2 commits into
base: master
from

Conversation

@GuillaumeGomez
Copy link
Member

commented May 13, 2019

I added a mechanism to have the possibility to add sub-settings (when clicking on the master, all the children take the same value) and I added it for types to get a more fine-tuned handling on which types you want to see their declaration or not by default.

A little screenshot of the setting page with the sub-settings:

Screenshot 2019-05-13 at 09 07 54

r? @Manishearth

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Can you prefix rustdoc PR titles with "rustdoc: "? Seeing them in the notifications is ambiguous and a bit confusing otherwise.

@GuillaumeGomez GuillaumeGomez changed the title Add sub settings [rustdoc] Add sub settings May 13, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Arf sorry, completely forgot. Updated the title!

@fbstj

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

does the code need to be general enough to cope with cases where a mixture of sub-setting choices needs to make the grouping setting into some indeterminate state (or into the "on" state)? the example screenshot might benefit from an indeterminate state?

maybe an ⓘ-style icon with more info about what clicking it does to the sub-settings and what clicking a sub-setting does to the master?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

@fbstj I don't know how to present it "nicely": the parent currently is the default if no value has been set to the child. If you change the state of the parent, all the children are updated as well.

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Ping from triage @Manishearth, this is waiting for your review

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

It's not clear to me we should have this feature, and I think @QuietMisdreavus shares this opinion. Was mostly waiting for her to make a decision on that.

r? @QuietMisdreavus

@QuietMisdreavus
Copy link
Member

left a comment

Other than the refactor to the settings code, i'm not totally convinced about this change. What's the motivation? This looks like a solution in search of a problem.

The reason i'm worried is that the settings page seems to have become a sort of catch-all solution to problems where a decision is split and no one is arguing strongly for either side. One of us chooses one version as the "default" and defers anyone else to the small, non-obvious button embedded onto every page. Sometimes, like this PR, it seems to be a change for change's sake, without adding something that creates a strong enough draw or solves a pressing problem.

I'm also worried about the added features in particular because of how we treat the auto-hide functionality in much the same way - a catch-all solution to a problem (long pages due to too much information) that creates new problems of its own (added load times on exceptionally long pages due to DOM manipulation). I feel like we're leaning too hard on the auto-hide functionality (and on the settings page) in terms of adding more run-time features for it.

Something to remember when adding features to rustdoc output is that we're not building a web app - we're building a static site generator. I think there's some design tension between these two conceptions at play here.

@@ -2067,7 +2067,7 @@ if (!DOMTokenList.prototype.remove) {
function autoCollapse(pageId, collapse) {
if (collapse) {
toggleAllDocs(pageId, true);
} else if (getCurrentValue("rustdoc-trait-implementations") !== "false") {
} else if (getCurrentValue("rustdoc-auto-hide-trait-implementations") !== "false") {

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus Jun 18, 2019

Member

Be aware that by changing the local storage keys that we look up, we're creating a situation on docs.rs (and sites like it that host docs made by multiple versions of rustdoc) where a setting made on one set of docs won't affect the other. Since we're adding a bunch more at the same time, it might be an understandable/okay change, but i'm not sure it's totally worthwhile.

Another problem with renaming existing local storage keys is that if someone had changed the setting, now they will be stuck with the default again and will need to go back and change. Should we try to have a kind of compatibility check that loads in a value from the previous key if the new one isn't set?

});
}
}
var showItemDeclarations = getCurrentValue("rustdoc-auto-hide-" + className);

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus Jun 18, 2019

Member

Ohhh, now i see why changing the names of the keys could be worthwhile. This might negate the previous comment.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@QuietMisdreavus For the settings rust code change, it's mainly to make it more easy to update. For the JS side, this feature allows to pick which kind of type definition you want to be expanded by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.