-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Dynamic play button based on no_run
property and used crates availability
#404
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
Conversation
crossDomain: true, | ||
dataType: "json", | ||
contentType: "application/json", | ||
success: function(response){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we do on error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we do on error?
Currently nothing. The play buttons will not appear which imho is a safer default than showing play buttons on content that might be unsupported by play.rlo.
I did not have a reasonable fallback scenario, do you have anything in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that is perfect 👍
Not showing the play button when the playground can not be reached makes a lot of sense :)
src/theme/book.js
Outdated
var txt = playpen_text(pre_block); | ||
var re = /extern\s+crate\s+([a-zA-Z_0-9]+)\s*;/g; | ||
var snippet_crates = []; | ||
while (item = re.exec(txt)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add braces for the while loop?
I tend to avoid brace-less control flow as it is error-prone when refactoring.
src/theme/book.js
Outdated
snippet_crates.push(item[1]); | ||
|
||
// check if all used crates are available on play.rust-lang.org | ||
var all_available = snippet_crates.every(elem => playground_crates.indexOf(elem) > -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we should use the arrow syntax (introduced in ES6) for compatibility reasons.
Sorry for the delay, I was quite busy lately. |
also minor refactor of clipboard handling TODO: - `no_run` support - test with ACE - disable play button with tooltip instead of hiding
- list of available crates is dynamically loaded from play.rust-lang.org - play button is enabled only if crates used in snippet are available on playground - ACE editor's play button is dynamically updated on each text change - `no_run` is honored by always disabling the play button - minor cleanups
5101b23
to
886f3b5
Compare
Both fixes pushed.
No problem. I'm the one out of touch these days :) |
hmm travis does not seam to be likely to finish anytime soon (and the change is js only anyway). I'll merge this one base on appveyor passing. |
fixes https://github.com/azerupi/mdBook/issues/396
no_run
is honored by always disabling the play buttonThis might not be ready for prime time as currently the play button is just hidden if both on
no_run
and when using crates unavailable on play.rust-lang.org. I would like to eventually just disable the play button with some visual cue (dim/color change) and tooltip describing the state (I guess that It'll wait for https://github.com/azerupi/mdBook/issues/388 ;)).But I wanted to push the WIP for review as soon as possible as I'll not have much free time on my hands for the next two weeks.
But OTOH it might be worth merging as it is anyway with later fix for nicer play button disabling.