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 genericity on rustdoc generated pages #29703

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
@GuillaumeGomez
Member

GuillaumeGomez commented Nov 9, 2015

r? @alexcrichton
cc @Gankro

This PR adds the possibility to switch style: generated doc link

image

I generate a file to include new css files directly before compiling librustdoc. However, a few questions remain:

  • Should I create a file which includes "basics" and only work on changes?
  • Is my change on Makefile file correct?

This is a first step before adding this change to rustbook as well.

@Gankro

This comment has been minimized.

Contributor

Gankro commented Nov 9, 2015

The screenshot is busted for me, idk github seems to be puking serving the file (I once got a blurred copy of what looked like a nice dark theme, now I get a blank image).

Is there a motivation for this addition? It's kinda cool, but we shouldn't necessarily add stuff just because it's cool.

Edit: fixed.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 9, 2015

Because this white documentation is really annoying and killing my eyes. But I didn't want to impose my preferences so I added the possibility to switch between the two styles.
@steveklabnik gave me his approval to do it so I did. :)

PS: hum, weird... What happened to the screenshot ? :'(

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 9, 2015

I'm also a little wary on adding multiple styles to documentation, is this common of other documentation generators? Perhaps the generation step could have a style, but the rendered step?

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 9, 2015

I find it better this way but I'm totally open to other proposition of course.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:style branch from d74ad25 to 98db899 Nov 9, 2015

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 9, 2015

I also updated the screenshot link. And improved the code a bit.

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Nov 10, 2015

So, I really dislike dark themes, but it's a very, very common feature request, so I did mention to @GuillaumeGomez that they should try to work this up, to see what it's like.

Usually, what people ask for is "how do I get a dark style".

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 10, 2015

If we're just trying to solve the questions of how to get a different style at all, aren't that what browser extensions are for? This should largely just boil down to a different stylesheet in theory.

I'd feel a little better about this if any other documentation generators had this sort of option, but I think I'd still prefer to remain consistent among all our docs. cc @brson, you've had thoughts about this in the past as well.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 10, 2015

Asking user to do it themselves by adding browser extensions is a bit annoying. Why not providing it ourselves if we can ? It doesn't take that much after all. :-/

@lwandrebeck

This comment has been minimized.

lwandrebeck commented Nov 10, 2015

Running Gnome 3 and apps in dark theme, I must admit being able to easily get doc in dark would save my eyes a bit 👍
@alexcrichton you do speak about browser extension, I’d need to check but I’m not that sure that Gnome Web (Epiphany) does support that. Thus, an in-doc dark (and others ?) theme would perfectly fit (at least my) some needs.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:style branch from 98db899 to ed30c2a Nov 10, 2015

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 10, 2015

I'd also like to precise that I want to implement it differently on rustbook : instead (or alongside, I really like the possibility to switch theme on the fly) of providing the possibility to switch the style while reading, I'd want to add the possibility to load a custom theme at book generation time.

I hope it was clear enough...

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 11, 2015

@alexcrichton I don't have a strong opinion here, but I think loading custom CSS is (imo) kind of a tall ask for ordinary humans.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 11, 2015

I do kinda agree that saying "you should use a browser extension" is quite a bit to ask, but on the other hand it also seems just as rare to request not only a different style from rustdoc but supporting multiple styles?

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 12, 2015

@alexcrichton: It's not rare for people to ask theme that fit them better, but it's rare that their request is granted, and that's a shame. I doesn't request much to do after all.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 12, 2015

Unfortunately an addition like this ends up not being small when viewed over the vector of time. Although this is not much to add now, this may have consequences like:

  • This creates an inconsistent experience when browsing Rust documentation. If your preference is an alternate theme then you have to set that for every rustdoc instance you view. Not only that, but most Rust "properties" have the same style (e.g. the home page and the std api docs), and you won't be able to change the theme of all of them.
  • Bug reports will be filed and we'll have to start asking which style everyone's using. This is just an added layer of friction which isn't always necessary.
  • Bug reports will specifically be filed against one theme or another requiring us to either collect them or actively fix problems in both styles.
  • New features in rustdoc will have to be design for all styles, not just one. It's already basically a herculean task keeping rustdoc up to date, much less keeping the style itself up to date.
@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 12, 2015

I don't agree with you:

  • It doesn't create an inconsistent experience but an alternative one. If it became the main theme, it'd be inconsistent but it's not the case.
  • I don't see how style (where only colors in css changed) can have bugs. And if bugs there are, they're also in the main theme.
  • Just like the previous point, if a bug is found on the dark theme, it certainly does exist for the main one.
  • This is an issue I thought about. But here, we're talking about modifying css, and nothing more. You make a change on the main theme, you report it on the dark one and it won't take much more time (but it'll take time, yes). For this point, you can just ping me to make the change and I'll gladly do it if it allows users to have a dark theme.
@vberger

This comment has been minimized.

Contributor

vberger commented Nov 12, 2015

Among the possibilities to reduce the burden of maintaining such a split, as long as the only change is the colors, wouldn't it be a possibility to split the CSS file in two ?

A file defining the structure of the page, and 2 small files for the colors (light.css / dark.css) ?

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 12, 2015

@vberger: Your explanation is way better than what I said:

Should I create a file which includes "basics" and only work on changes?

Thanks for it. :)

@@ -598,7 +598,7 @@ ALL_TARGET_RULES = $(foreach target,$(CFG_TARGET), \
$(foreach host,$(CFG_HOST), \
all-target-$(target)-host-$(host)))
all: $(ALL_TARGET_RULES) $(GENERATED) docs
all: $(shell $(S)src/etc/generate_styles.py) $(ALL_TARGET_RULES) $(GENERATED) docs

This comment has been minimized.

@wthrowe

wthrowe Nov 12, 2015

Contributor

Target dependencies are evaluated at parse time, so this is going to cause generate_styles.py to be run every time make is invoked. It should be moved to a recipe somewhere.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Nov 12, 2015

Member

I put it there in order to make it work. I couldn't figure out where was the right place but if you know it, please tell me the secret ! :)

This comment has been minimized.

@wthrowe

wthrowe Nov 12, 2015

Contributor

I believe $(S)src/librustdoc/html/styles.rs should have its own target, much like how llvmdeps.rs used to be handled. The appropriate stamp file (stamp.rustdoc, I think) should depend on it.

I'm not sure if generating the file in src is appropriate or if it should be somewhere in the target output directory. If the latter it should probably look like the current llvmdeps.rs code. If it does end up in src it should be presumably be added to .gitignore.

@brson

This comment has been minimized.

Contributor

brson commented Nov 13, 2015

I'm definitely on board with being able to customize the generated style.

I too am a little wary of being able to switch styles at runtime. As someone who likes a nice dark background though, I also understand the appeal. (As an aside, I still really want the rustonomicon to be dark by default.) I always want to resist the urge to add more options to distract people with.

Does this patch do both and can it be split into two prs? Since adding the dark theme to the live docs is going to require a lot of consideration anyway, I think it's reasonable to split this up as possible.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 13, 2015

@brson: Like explained by @vberger, I'll create a common file with basics of rustdoc style and then just leave theme changes in both main.css and dark.css. So this PR will mainly add the possibility to switch theme. I'll provide a live rustdoc demo as soon as possible.

Thanks for your support ! :)

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:style branch from ed30c2a to 89ccd19 Nov 14, 2015

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 14, 2015

I splitted the css files and added styles.rs to gitignore file. I'll create the online rustdoc tomorrow.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:style branch 2 times, most recently from a58167a to e59ed42 Nov 14, 2015

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 14, 2015

And here is the generated std doc.

@brson

This comment has been minimized.

Contributor

brson commented Nov 17, 2015

Well, I'm conflicted now. The thing I was more convinced of (customizable static themes) is not what others want or this provides. So I guess the main question is whether we want to let people switch dynamically to a dark theme. If so, then whether the provided dark theme is suitably attractive and complementary to the light, and what this means for our other docs.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 17, 2015

@brson: For rustbook, I have two things in mind:

  • Just like rustdoc, wwe change the style dynamically.
  • We specify the style when we build the book.

And... Is there other documentation ?

@brson

This comment has been minimized.

Contributor

brson commented Nov 17, 2015

@GuillaumeGomez Why does rustbook specify the style when building, and rustdoc doesn't?

Other docs include the doc index and error index, the website, and rbe which seems to still use gitbook. None feel too important, though it would be nice to get rbe onto rustbook.

Does the way you are storing theme preferences allow one setting to be shared by all our rustbook and rustdoc projects across subdomains?

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 17, 2015

For rustbook, it's because we generally build the book for ourselves. I actually want to add both options.

For rustdoc, we impose it. And you'll notice that the change is very little for those who don't want the dark theme.

For other docs, I guess providing an alternative, just like for rustdoc, seems to be the best solution.

@mdinger

This comment has been minimized.

Contributor

mdinger commented Nov 17, 2015

I'm opposed to moving rbe to rustbook until multiple themes land on rustbook and frankly, mdbook seems to get much more effort put into it than rustbook and I'd probably port to that first. It already has the theme support.

Aside from that, there are a few things that prevent porting rbe (there was another but I can't remember what it is. Search probably wouldn't be a blocker but losing it wouldn't be good):

  • cargo needs to expose what's in the Cargo.toml via some api. This would allow the structure of the examples to be used to generate the structure of the TOC (which is basically what it does already without cargo)
  • a plugin or something needs to be setup to allow the embedded playpen. Hopefully it'll be better than the current one because mucking around with the output as rbe does makes errors much more difficult to interpret than in the playpen.
  • we lose search by moving away from gitbook currently. Search is pretty useful. mdbook is planning to add it but who knows when it'll land.
@brson

This comment has been minimized.

Contributor

brson commented Nov 18, 2015

For rustbook, it's because we generally build the book for ourselves

@GuillaumeGomez This still isn't clear to me. Ourselves meaning 'for personal use' as opposed to 'for publication'?

mdbook seems to get much more effort put into it than rustbook and I'd probably port to that first. It already has the theme support.

@mdinger Hm, it does look better-maintained. If we moved the books out of the repo we could move them all to mdbook. We should be consolidating our tools long-term and I don't want to be on 2 (or 3!) *books.

@Gankro

This comment has been minimized.

Contributor

Gankro commented Nov 18, 2015

@brson I think the key insight here is that rustbook is generally invoked "directly" (although ultimately automated away by build scripts), but rustdoc is generally invoked indirectly (through cargo doc or similar).

@mdinger

This comment has been minimized.

Contributor

mdinger commented Nov 18, 2015

@brson I'm all for consolidation but for rbe's needs, I don't think the tooling is ready. Other books like the book and the rustonomicon probably don't have such strict requirements.

Having the native tools external to rust should be a boon for usability too (that was day 2 of him trying to build it. Here's day 1).

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:style branch from 1ac83a2 to 9affa0a Nov 18, 2015

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 18, 2015

I changed the way the style is changed while the DOM content is loaded. The flash shouldn't happen anymore.

@mdinger

This comment has been minimized.

Contributor

mdinger commented Nov 19, 2015

Deprecated and unstable look better. I'm sensing kinda a tron theme ha.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 19, 2015

A tron theme ? Not flashy enough. And the blue should dominate more. So definitely not tron (too bad, I like tron ^^).

@brson

This comment has been minimized.

Contributor

brson commented Nov 24, 2015

Although this is well done, and there are strong advocates for it, I am going to close it without merging. The main reason is that forces us to consider two visual designs for all Rust documentation, which is presently created through 4 or 5 different tools. Furthermore, even if we were to follow through with overhauling all existing documentation in light of this concern, we will have to deal with the dual-design precedent when we overhaul and consolidate our visual designs in the future.

Thanks @GuillaumeGomez.

@brson brson closed this Nov 24, 2015

@mdinger

This comment has been minimized.

Contributor

mdinger commented Nov 24, 2015

In that case some kind of placeholder bug which details the problems blocking this from merging should be created. Then progress can be made towards this in the future, at the very least.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 24, 2015

I was really hoping to finally have the official docs with dark style. Like said on IRC, I don't mind adding this dark theme everywhere and maintain it if needed. But this light theme is really uncomfortable.

@brson

This comment has been minimized.

Contributor

brson commented Nov 24, 2015

@mdinger I didn't mean to imply that there is a path forward for adding dark themes. It is something though we should reconsider in the future when doing a concerted website redesign.

@liigo

This comment has been minimized.

Contributor

liigo commented Nov 25, 2015

@mdinger

This comment has been minimized.

Contributor

mdinger commented Nov 25, 2015

@brson Oh. Thanks for clarifying. I understood it to mean that the reason this isn't accepted is because the implementation is via 4-5 different tools and so it needs to be consolidated. Not because light and dark isn't part of the current scheme.

@brson brson referenced this pull request Nov 25, 2015

Closed

Style hierarchy #30045

@PersonneHasard1995

This comment has been minimized.

PersonneHasard1995 commented Dec 10, 2015

For me to have this dark style is really a good thing. Let me explain we all have differents style, when i saw this new dark style it was a really pleasure for my eyes because the white style are hurting each time i'm seing my screen even with the minimal brightness and it distorts my reading because my eyes become tired. With the dark style it would be a really pleasure to read the documentation.

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:style branch Nov 24, 2017

@najamelan

This comment has been minimized.

najamelan commented Nov 29, 2017

I know that this is a bit old, but if I understand it right, we have to do this currently. It's not very ergonomic to have to find that page, pull out that cargo command every time we generate docs (and find that the css is outdated and should be modified further, and needs to be put either in every crate or we need to have the command take an absolute path).

I would still like to see something like cargo doc --open --dark supported out of the box.

I read Alex worries about extra work and maintenance, but it could be quite simple. Have a text file which maps 3 fields (colorname:light:dark) and have an automated build step which will use regex replace to plug the right colors in your themes. Other improvements can be made but are optional, eg. separate color related styles from layout styles, or use scss to actually have variable names for the colors, and have a dark and a light list of variable names in 2 files.
Another option is to generate both anyways and let the user change it on the page with a button like duckduckgo does it. Where far not as good as having stuff automatically dark all the time, we might be reading documentation for quite a long time, which makes clicking a button once per opening the docs a fair improvement over the current state of affairs.
thanks for all the great work from the rust team though, I understand it's work.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 29, 2017

Too complicated. I told it a few times already but I'll add alternative themes "soon". You'll be able to switch themes directly. At first I'll just put dark as an extra one and we'll see later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment