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.

Sign up for GitHub

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

Jump to bottom

add kbd style tag to main.css in rustdoc #46938

Merged
merged 4 commits into from
Jan 21, 2018
Merged

add kbd style tag to main.css in rustdoc #46938

merged 4 commits into from
Jan 21, 2018

Conversation

hellow554
Copy link
Contributor

Added css style for kbd tags so they actually look like keys.
Result preview and discussion was going on in #46900 .

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@GuillaumeGomez
Copy link
Member

CSS seems good. Can you add a screenshot with a key in the middle of a text please?

GuillaumeGomez
GuillaumeGomez reviewed Dec 22, 2017
View reviewed changes
src/librustdoc/html/static/rustdoc.css Outdated
line-height: 10px;
color: #444d56;
vertical-align: middle;
background-color: #fafbfc;
Copy link
Member

Choose a reason for hiding this comment

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

Oh forgot this: please put all color rules into the main.css. The point is to make easier the add of multiple themes by just changing one CSS file instead of several.

@GuillaumeGomez
Copy link
Member

Also, as long as the discussion issue isn't resolved, this PR's merge will be frozen. Being the rustdoc's style only maintainer (or not so far, sorry if I missed someone...), I'm really opposed to this add (I add the explanation once again) since it doesn't add something useful for rustdoc rendering but an ease for users (or maybe just user, singular). This isn't the point of this file. It's already really difficult to maintain and I don't want to increase the burden for something mostly useless.

If users want, they can add the missing style by themselves without issues. I added such options in rustdoc (I'm referring to --extend-css). Please use it instead.

@hellow554
Copy link
Contributor Author

I added the colors into main css, and here is a screenshot

screenshot

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Dec 22, 2017
@alexcrichton
Copy link
Member

ping @GuillaumeGomez, looks likt his may be ready for another inspection!

GuillaumeGomez
GuillaumeGomez reviewed Jan 4, 2018
View reviewed changes
src/librustdoc/html/static/styles/main.css Outdated
border-bottom-color: #c6cbd1;
box-shadow-color: #c6cbd1;
}

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the last, empty line?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please!

@GuillaumeGomez
Copy link
Member

The change is incomplete. I'll quote myself (hehe):

If the PR does the switch (aka removing dt tag and style). Then it's good for me. As long as it's used by rustdoc, no problem.

So the style change seems good, but I can't accept the PR as is, sorry. Just a bit more to do and we're good to go!

@hellow554
Copy link
Contributor Author

Sorry, I was on vacation and had no time doing stuff. I will tackle it this weekend, hopefully

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jan 5, 2018 •
edited
Loading

No need to hurry or worry, and thanks!

@carols10cents carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2018
@hellow554
extended dt with kbd tags
38e5d30 
removed styling of dt tages, which would make them look like keys and
added <kbd> tag inside of dt tag.
Added css style for kbd and removed some stylings for dt
@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2018
@GuillaumeGomez
Copy link
Member

Just one last thing (two with the extra empty line at the end of the file): please change all your whitespaces into tabulations into the CSS files.

@hellow554
Copy link
Contributor Author

Done

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jan 18, 2018

📌 Commit 0c946c0 has been approved by GuillaumeGomez

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 19, 2018
@GuillaumeGomez
Rollup merge of rust-lang#46938 - hellow554:rustdoc-kbd-style, r=Guil…
6e9cbac 
…laumeGomez

add kbd style tag to main.css in rustdoc

Added css style for kbd tags so they actually look like keys.
Result preview and discussion was going on in rust-lang#46900 .
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 19, 2018
Closed
bors added a commit that referenced this pull request Jan 19, 2018
@bors
Auto merge of #47572 - GuillaumeGomez:rollup, r=GuillaumeGomez
a7b8622 
Rollup of 8 pull requests

- Successful merges: #46938, #47334, #47420, #47508, #47510, #47512, #47535, #47559
- Failed merges:
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 20, 2018
@GuillaumeGomez
Rollup merge of rust-lang#46938 - hellow554:rustdoc-kbd-style, r=Guil…
fe811eb 
…laumeGomez

add kbd style tag to main.css in rustdoc

Added css style for kbd tags so they actually look like keys.
Result preview and discussion was going on in rust-lang#46900 .
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 20, 2018
Merged
bors added a commit that referenced this pull request Jan 21, 2018
@bors
Auto merge of #47622 - GuillaumeGomez:rollup, r=GuillaumeGomez
8d3e93b 
Rollup of 10 pull requests

- Successful merges: #46938, #47193, #47508, #47510, #47532, #47535, #47559, #47568, #47573, #47578
- Failed merges:
@bors bors merged commit 0c946c0 into rust-lang:master Jan 21, 2018
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hellow554 @rust-highfive @GuillaumeGomez @alexcrichton @bors @kennytm @carols10cents