Fixes run button appearing when it shouldn't #36637

GuillaumeGomez · 2016-09-21T22:25:36Z

bluss · 2016-09-22T09:02:22Z

Thanks!

frewsxcv · 2016-09-22T23:34:53Z

src/librustdoc/html/static/playpen.js

@@ -15,6 +15,11 @@ document.addEventListener('DOMContentLoaded', function() {
    'use strict';

    if (!window.playgroundUrl) {
+        var run_buttons = document.querySelectorAll(".test-arrow");


Very very minor, but it looks like naming conventions in the rest of this file use camel case, not snake case.

It's javascript, who cares.

I think there is some value in caring about keeping consistent with naming conventions. It's not clear to me why we shouldn't care about JavaScript.

I used trollface to make sure people understand it was a troll. I failed :-/

I'll update later on.

GuillaumeGomez · 2016-09-23T22:09:54Z

Updated.

bluss · 2016-09-29T22:22:52Z

I tried this locally and this does not fix the issue. playpen.js is not used at all for a crate without playgroundUrl configured.

See first and last lines in this range:

rust/src/librustdoc/html/layout.rs

Lines 143 to 181 in 289f3a4

    
               {play_js} 
        
               <script defer src="{root_path}search-index.js"></script> 
        
           </body> 
        
           </html>"##, 
        
               css_extension = if css_file_extension { 
        
                   format!("<link rel=\"stylesheet\" type=\"text/css\" href=\"{root_path}theme.css\">", 
        
                           root_path = page.root_path) 
        
               } else { 
        
                   "".to_owned() 
        
               }, 
        
               content   = *t, 
        
               root_path = page.root_path, 
        
               css_class = page.css_class, 
        
               logo      = if layout.logo.is_empty() { 
        
                   "".to_string() 
        
               } else { 
        
                   format!("<a href='{}{}/index.html'>\ 
        
                            <img src='{}' alt='logo' width='100'></a>", 
        
                           page.root_path, layout.krate, 
        
                           layout.logo) 
        
               }, 
        
               title     = page.title, 
        
               description = page.description, 
        
               keywords = page.keywords, 
        
               favicon   = if layout.favicon.is_empty() { 
        
                   "".to_string() 
        
               } else { 
        
                   format!(r#"<link rel="shortcut icon" href="{}">"#, layout.favicon) 
        
               }, 
        
               in_header = layout.external_html.in_header, 
        
               before_content = layout.external_html.before_content, 
        
               after_content = layout.external_html.after_content, 
        
               sidebar   = *sidebar, 
        
               krate     = layout.krate, 
        
               play_url  = layout.playground_url, 
        
               play_js   = if layout.playground_url.is_empty() { 
        
                   "".to_string() 
        
               } else { 
        
                   format!(r#"<script src="{}playpen.js"></script>"#, page.root_path)

GuillaumeGomez · 2016-09-30T09:46:28Z

Strange. I'll retest once again. Thanks for your feedback @bluss!

GuillaumeGomez · 2016-10-03T19:56:50Z

@bluss: Thanks for pointing me to this! Can you test it again please?

bluss · 2016-10-04T11:29:58Z

@GuillaumeGomez The fix looks like it will work to me. I would probably just put it in main.js, that simplifies things. Unfortunately I'd have to rebuild llvm to test the rustdoc build, and I don't have the time to do it.

GuillaumeGomez · 2016-10-04T15:51:01Z

I don't want it all the time, I just want it in case playpen.js isn't present.

GuillaumeGomez · 2016-10-05T09:19:22Z

Hum... I'll create another js file instead.

GuillaumeGomez · 2016-10-06T10:02:46Z

Ok, seems like it's all good now.

steveklabnik · 2016-10-06T17:10:35Z

@bluss what do you think of this PR now?

bluss · 2016-10-07T14:01:42Z

It looks like it will work fine. But since both extra.js, and playpen.js have the same snippet to remove the run buttons, that part, it might as well be in main.js instead?

GuillaumeGomez · 2016-10-07T22:03:14Z

@bluss: No, the point is to avoid putting too much watchers. One by button is already enough. If you put this part in main.js, it means we'll have it duplicated since playpen.js (in case it'll be there) will still put the watcher on it.

I hope I wrote was clear enough. :-/

bluss · 2016-10-08T11:56:26Z

Oh I see, that makes sense.

bluss · 2016-10-08T11:58:44Z

@bors r+

Let's go.

bors · 2016-10-08T11:58:45Z

📌 Commit 8983d1e has been approved by bluss

GuillaumeGomez · 2016-10-08T13:02:02Z

@bors: rollup

bors · 2016-10-08T16:33:57Z

⌛ Testing commit 8983d1e with merge fcc354f...

@steveklabnik

Fixes run button appearing when it shouldn't Fixes #36621. r? @steveklabnik

bors · 2016-10-08T19:36:10Z

💔 Test failed - auto-mac-32-opt

alexcrichton · 2016-10-08T19:40:20Z

@bors: retry

On Sat, Oct 8, 2016 at 12:36 PM, bors notifications@github.com wrote:

💔 Test failed - auto-mac-32-opt
https://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/10751

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#36637 (comment), or mute
the thread
/~https://github.com/notifications/unsubscribe-auth/AAD95JwRea2S2iHajCDiqOzIdDl_gSxQks5qx_CtgaJpZM4KDWCb
.

bors · 2016-10-09T02:42:56Z

⌛ Testing commit 8983d1e with merge b98cc35...

@steveklabnik

Fixes run button appearing when it shouldn't Fixes #36621. r? @steveklabnik

bors · 2016-10-09T07:35:54Z

rust-highfive assigned steveklabnik Sep 21, 2016

frewsxcv reviewed Sep 22, 2016

View reviewed changes

GuillaumeGomez force-pushed the fix_run_button branch from 214461d to 7a31d3f Compare September 23, 2016 22:09

frewsxcv approved these changes Sep 29, 2016

View reviewed changes

GuillaumeGomez force-pushed the fix_run_button branch from 7a31d3f to db9b846 Compare October 3, 2016 19:56

GuillaumeGomez force-pushed the fix_run_button branch from db9b846 to 0c3ad61 Compare October 4, 2016 17:15

Fixes run button appearing when it shouldn't

8983d1e

GuillaumeGomez force-pushed the fix_run_button branch from 0c3ad61 to 8983d1e Compare October 5, 2016 19:15

bors added a commit that referenced this pull request Oct 8, 2016

Auto merge of #36637 - GuillaumeGomez:fix_run_button, r=bluss

fcc354f

Fixes run button appearing when it shouldn't Fixes #36621. r? @steveklabnik

bors added a commit that referenced this pull request Oct 9, 2016

Auto merge of #36637 - GuillaumeGomez:fix_run_button, r=bluss

b98cc35

Fixes run button appearing when it shouldn't Fixes #36621. r? @steveklabnik

bors merged commit 8983d1e into rust-lang:master Oct 9, 2016

GuillaumeGomez deleted the fix_run_button branch October 9, 2016 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes run button appearing when it shouldn't #36637

Fixes run button appearing when it shouldn't #36637

GuillaumeGomez commented Sep 21, 2016

bluss commented Sep 22, 2016

frewsxcv Sep 22, 2016

GuillaumeGomez Sep 23, 2016

frewsxcv Sep 23, 2016

GuillaumeGomez Sep 23, 2016

GuillaumeGomez commented Sep 23, 2016

bluss commented Sep 29, 2016

GuillaumeGomez commented Sep 30, 2016

GuillaumeGomez commented Oct 3, 2016

bluss commented Oct 4, 2016

GuillaumeGomez commented Oct 4, 2016

GuillaumeGomez commented Oct 5, 2016

GuillaumeGomez commented Oct 6, 2016

steveklabnik commented Oct 6, 2016

bluss commented Oct 7, 2016

GuillaumeGomez commented Oct 7, 2016 •

edited

Loading

bluss commented Oct 8, 2016

bluss commented Oct 8, 2016

bors commented Oct 8, 2016

GuillaumeGomez commented Oct 8, 2016

bors commented Oct 8, 2016

bors commented Oct 8, 2016

alexcrichton commented Oct 8, 2016

bors commented Oct 9, 2016

bors commented Oct 9, 2016

Fixes run button appearing when it shouldn't #36637

Fixes run button appearing when it shouldn't #36637

Conversation

GuillaumeGomez commented Sep 21, 2016

bluss commented Sep 22, 2016

frewsxcv Sep 22, 2016

Choose a reason for hiding this comment

GuillaumeGomez Sep 23, 2016

Choose a reason for hiding this comment

frewsxcv Sep 23, 2016

Choose a reason for hiding this comment

GuillaumeGomez Sep 23, 2016

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 23, 2016

bluss commented Sep 29, 2016

GuillaumeGomez commented Sep 30, 2016

GuillaumeGomez commented Oct 3, 2016

bluss commented Oct 4, 2016

GuillaumeGomez commented Oct 4, 2016

GuillaumeGomez commented Oct 5, 2016

GuillaumeGomez commented Oct 6, 2016

steveklabnik commented Oct 6, 2016

bluss commented Oct 7, 2016

GuillaumeGomez commented Oct 7, 2016 • edited Loading

bluss commented Oct 8, 2016

bluss commented Oct 8, 2016

bors commented Oct 8, 2016

GuillaumeGomez commented Oct 8, 2016

bors commented Oct 8, 2016

bors commented Oct 8, 2016

alexcrichton commented Oct 8, 2016

bors commented Oct 9, 2016

bors commented Oct 9, 2016

GuillaumeGomez commented Oct 7, 2016 •

edited

Loading