Codeblock color by GuillaumeGomez · Pull Request #44397 · rust-lang/rust

GuillaumeGomez · 2017-09-07T20:10:04Z

This screenshot has been generated from:

/// foo////// ```compile_fail/// foo();/// ```////// ```ignore/// goo();/// ```////// ```/// let x = 0;/// ```pubfnbar() -> usize{2}

r? @QuietMisdreavus
cc @rust-lang/docs

QuietMisdreavus · 2017-09-07T20:19:59Z

I'm generally +1 on this, though I'd also like to see what the rest of the docs team thinks.

Technically this is gated on #43949, since it makes the same change to the compile_fail flag. If you put back the nightly check for compile_fail then that should be fine.

chordowl · 2017-09-07T20:20:35Z

🚲🏚️: I'm wondering if there's a nicer way to phrase this than "so be extra careful" (since it doesn't compile anyway), but: I don't have a better suggestion than just leaving that part off so I think it's fine as-is, too.

Something like "Warning: This code will not compile!", maybe?

estebank · 2017-09-07T20:46:31Z

@GuillaumeGomez could you check how it looks if only the border-left is given the proposed colors? I feel that the current design drives too much attention to the emptiness on the right hand side of the code block.

</bikeshedding>

QuietMisdreavus · 2017-09-07T20:55:04Z

travis failures:

[00:03:47] tidy error: /checkout/src/test/rustdoc/codeblock-title.rs:13: line longer than 100 chars
[00:03:47] tidy error: /checkout/src/test/rustdoc/codeblock-title.rs:14: line longer than 100 chars
[00:03:47] tidy error: /checkout/src/test/rustdoc/codeblock-title.rs:22: unexplained "```ignore" doctest; try one:
[00:03:47] [00:03:47] * make the test actually pass, by adding necessary imports and declarations, or
[00:03:47] * use "```text", if the code is not Rust code, or
[00:03:47] * use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or
[00:03:47] * use "```should_panic", if the code is expected to fail at run time, or
[00:03:47] * use "```no_run", if the code should type-check but not necessary linkable/runnable, or
[00:03:47] * explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.

GuillaumeGomez · 2017-09-08T08:58:35Z

@QuietMisdreavus: Oh, forgot to remove the 100 columns limit on the test.
@estebank: I think you're right, it attracts attention too much. Let's see with just left. ;)

GuillaumeGomez · 2017-09-08T14:21:30Z

frewsxcv · 2017-09-08T14:31:24Z

I'm +0 on this right now. I think the visual indicators aren't subtle enough and draw too much attention to the code block. I think a symbol like this could probably be sufficient to warn the user instead of incorporating such vibrant colors, but I'm not really sure what the best approach is here.

QuietMisdreavus · 2017-09-08T14:40:10Z

That's a good point. Without another kind of indicator on "good" code blocks, it does make the "bad" ones stick out a bit much. I like the idea of using an indicator character like the one you linked; would it be possible to sketch that one out?

est31 · 2017-09-08T15:07:59Z

What about a red ⓕ for should fail and a yellow ⓘ for ignored? I think though that snippets you want to ignore shouldn't get be singled out in the rendered documentation as who tells me in the first place that the other snippets test successfully?

GuillaumeGomez · 2017-09-08T15:09:42Z

I proposed to add a gren border for the "ok" code blocks and to reduce the outstanding of the other blocks. I don't like the character proposition because it's just another one in the middle of a lot.

steveklabnik · 2017-09-08T17:03:02Z

This would fix #20524, those comments should be read as well. Note that the proposed screenshot uses just the left border, like @estebank suggested above.

I also like the idea of symbols.

Basically, this is a huge 👍 from me generally, but I do think working out the details here would be important. And it'd be nice if we could only gate part of it on compile_fail, ignore alone would be a big improvement.

estebank · 2017-09-08T17:40:54Z

What about expanding the current proposal with that has been mentioned already?

If using the unicode char ⓕ or any other icon, I'd make sure to add a tooltip for it.

QuietMisdreavus · 2017-09-08T21:26:49Z

On IRC, i proposed making the border appear on hover, rather than at all times. That way, the border doesn't distract unless someone has their cursor over the block, like if they're trying to copy the code out. We could even add in the "help" cursor so people are more likely to wait for the tooltip to appear.

The linked issue also mentions making sure that we provide sufficient accessibility hints to make sure that users with screen readers or other methods also get the notes.

GuillaumeGomez · 2017-09-09T13:37:21Z

Ok so new output:

GuillaumeGomez · 2017-09-10T12:05:38Z

Updated.

QuietMisdreavus · 2017-09-11T19:32:23Z

We talked about it on IRC, and a couple comments came out:

The little glyph isn't exactly what was asked for. The original request was to use a circle-f for compile_fail examples, and a circle-i for ignore examples, but the PR as-is puts a circle-f on all of them. I'd personally just stick with the same ⚠ warning glyph we use when listing unsafe functions, but i don't know what everyone else in this thread thinks about that.
Right now, the PR stabilizes the compile_fail flag, even though Compile fail stable #43949 is meant to do that. After talking about it on IRC, we decided to take those changes out of this PR, and explicitly gate it on Compile fail stable #43949. @GuillaumeGomez didn't want to land this change until compile_fail was fully stable.

steveklabnik · 2017-09-11T19:40:25Z

but i don't know what everyone else in this thread thinks about that.

Seems good to me!

@GuillaumeGomez didn't want to land this change until compile_fail was fully stable.

👍

GuillaumeGomez · 2017-09-11T20:12:35Z

QuietMisdreavus · 2017-09-11T20:25:51Z

Would be nice to say something like (checking-rendering-of-ignore-blocks) instead of (tidy) here. That way it explicitly says why it's there, rather than just putting something there to make tidy happy. >_>

Haha. I'll think about it. :p

QuietMisdreavus · 2017-09-11T20:26:31Z

This is the glyph i was talking about. Changing that circle-f with the warning symbol is what i was suggesting earlier.

QuietMisdreavus · 2017-09-11T22:18:54Z

r=me once #43949 lands

GuillaumeGomez · 2017-09-15T13:01:01Z

@bors: r=QuietMisdreavus

bors · 2017-09-15T13:01:02Z

📌 Commit 79f888d has been approved by QuietMisdreavus

GuillaumeGomez · 2017-09-15T14:47:42Z

@bors: rollup

@QuietMisdreavus

…uietMisdreavus Codeblock color <img width="1440" alt="screen shot 2017-09-07 at 21 53 58" src="https://user-images.githubusercontent.com/3050060/30183045-4319108e-9419-11e7-98da-da54952cab37.png"> This screenshot has been generated from: ```rust /// foo /// /// ```compile_fail /// foo(); /// ``` /// /// ```ignore /// goo(); /// ``` /// /// ``` /// let x = 0; /// ``` pub fn bar() -> usize { 2 } ``` r? @QuietMisdreavus cc @rust-lang/docs

@QuietMisdreavus

…uietMisdreavus Codeblock color <img width="1440" alt="screen shot 2017-09-07 at 21 53 58" src="https://user-images.githubusercontent.com/3050060/30183045-4319108e-9419-11e7-98da-da54952cab37.png"> This screenshot has been generated from: ```rust /// foo /// /// ```compile_fail /// foo(); /// ``` /// /// ```ignore /// goo(); /// ``` /// /// ``` /// let x = 0; /// ``` pub fn bar() -> usize { 2 } ``` r? @QuietMisdreavus cc @rust-lang/docs

@QuietMisdreavus

…uietMisdreavus Codeblock color <img width="1440" alt="screen shot 2017-09-07 at 21 53 58" src="https://user-images.githubusercontent.com/3050060/30183045-4319108e-9419-11e7-98da-da54952cab37.png"> This screenshot has been generated from: ```rust /// foo /// /// ```compile_fail /// foo(); /// ``` /// /// ```ignore /// goo(); /// ``` /// /// ``` /// let x = 0; /// ``` pub fn bar() -> usize { 2 } ``` r? @QuietMisdreavus cc @rust-lang/docs

@QuietMisdreavus

…uietMisdreavus Codeblock color <img width="1440" alt="screen shot 2017-09-07 at 21 53 58" src="https://user-images.githubusercontent.com/3050060/30183045-4319108e-9419-11e7-98da-da54952cab37.png"> This screenshot has been generated from: ```rust /// foo /// /// ```compile_fail /// foo(); /// ``` /// /// ```ignore /// goo(); /// ``` /// /// ``` /// let x = 0; /// ``` pub fn bar() -> usize { 2 } ``` r? @QuietMisdreavus cc @rust-lang/docs

@QuietMisdreavus

…uietMisdreavus Codeblock color <img width="1440" alt="screen shot 2017-09-07 at 21 53 58" src="https://user-images.githubusercontent.com/3050060/30183045-4319108e-9419-11e7-98da-da54952cab37.png"> This screenshot has been generated from: ```rust /// foo /// /// ```compile_fail /// foo(); /// ``` /// /// ```ignore /// goo(); /// ``` /// /// ``` /// let x = 0; /// ``` pub fn bar() -> usize { 2 } ``` r? @QuietMisdreavus cc @rust-lang/docs

@QuietMisdreavus

…uietMisdreavus Codeblock color <img width="1440" alt="screen shot 2017-09-07 at 21 53 58" src="https://user-images.githubusercontent.com/3050060/30183045-4319108e-9419-11e7-98da-da54952cab37.png"> This screenshot has been generated from: ```rust /// foo /// /// ```compile_fail /// foo(); /// ``` /// /// ```ignore /// goo(); /// ``` /// /// ``` /// let x = 0; /// ``` pub fn bar() -> usize { 2 } ``` r? @QuietMisdreavus cc @rust-lang/docs

Rollup of 17 pull requests - Successful merges: #44073, #44088, #44381, #44397, #44509, #44533, #44549, #44553, #44562, #44567, #44595, #44604, #44617, #44622, #44630, #44639, #44647 - Failed merges:

rust-highfive assigned QuietMisdreavus Sep 7, 2017

chordowl reviewed Sep 7, 2017
View reviewed changes

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2017

GuillaumeGomez force-pushed the codeblock-color branch from 5e70325 to 30912c4Compare September 8, 2017 14:18

GuillaumeGomez force-pushed the codeblock-color branch from e73a380 to b22043aCompare September 9, 2017 13:36

GuillaumeGomez force-pushed the codeblock-color branch 2 times, most recently from 822775b to 3a256cfCompare September 9, 2017 21:06

GuillaumeGomez force-pushed the codeblock-color branch from 3a256cf to 797286dCompare September 11, 2017 20:11

QuietMisdreavus reviewed Sep 11, 2017
View reviewed changes

GuillaumeGomez added 3 commits September 11, 2017 22:31

Add class for codeblocks
a095ee4

add test
9c12e5d

Add arrow and improve display
79f888d

GuillaumeGomez force-pushed the codeblock-color branch from 797286d to 79f888dCompare September 11, 2017 20:31

GuillaumeGomez mentioned this pull request Sep 15, 2017
Rollup of 9 pull requests #44599
Closed

alexcrichton mentioned this pull request Sep 16, 2017
Rollup of 27 pull requests #44621
Closed

frewsxcv mentioned this pull request Sep 16, 2017
Rollup of 25 pull requests #44623
Closed

TimNN mentioned this pull request Sep 17, 2017
Rollup of 19 pull requests #44649
Closed

TimNN mentioned this pull request Sep 17, 2017
Rollup of 18 pull requests #44652
Closed

TimNN mentioned this pull request Sep 17, 2017
Rollup of 17 pull requests #44654
Merged

bors merged commit 79f888d into rust-lang:masterSep 17, 2017

GuillaumeGomez deleted the codeblock-color branch September 17, 2017 21:55

est31 mentioned this pull request Sep 30, 2017
Warn more gently on ignored example code #44927
Closed

kzys mentioned this pull request Sep 13, 2018
Consider indicating which code blocks are expected to compile and which are not in the guide(s) #20524
Closed

Uh oh!

Uh oh!

Conversation

GuillaumeGomez commented Sep 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuietMisdreavus commented Sep 7, 2017

Uh oh!

chordowlSep 7, 2017

Choose a reason for hiding this comment

Uh oh!

QuietMisdreavusSep 7, 2017

Choose a reason for hiding this comment

Uh oh!

estebank commented Sep 7, 2017

Uh oh!

QuietMisdreavus commented Sep 7, 2017

Uh oh!

GuillaumeGomez commented Sep 8, 2017

Uh oh!

GuillaumeGomez commented Sep 8, 2017

Uh oh!

frewsxcv commented Sep 8, 2017

Uh oh!

QuietMisdreavus commented Sep 8, 2017

Uh oh!

est31 commented Sep 8, 2017

Uh oh!

GuillaumeGomez commented Sep 8, 2017

Uh oh!

steveklabnik commented Sep 8, 2017

Uh oh!

estebank commented Sep 8, 2017

Uh oh!

QuietMisdreavus commented Sep 8, 2017

Uh oh!

GuillaumeGomez commented Sep 9, 2017

Uh oh!

GuillaumeGomez commented Sep 10, 2017

Uh oh!

QuietMisdreavus commented Sep 11, 2017

Uh oh!

steveklabnik commented Sep 11, 2017

Uh oh!

GuillaumeGomez commented Sep 11, 2017

Uh oh!

QuietMisdreavusSep 11, 2017

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomezSep 12, 2017

Choose a reason for hiding this comment

Uh oh!

QuietMisdreavusSep 11, 2017

Choose a reason for hiding this comment

Uh oh!

QuietMisdreavus commented Sep 11, 2017

Uh oh!

GuillaumeGomez commented Sep 15, 2017

Uh oh!

bors commented Sep 15, 2017

Uh oh!

GuillaumeGomez commented Sep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

GuillaumeGomez commented Sep 7, 2017•
edited
Loading