-
Notifications
You must be signed in to change notification settings - Fork 81
Conversation
/cc @brson |
@thehydroimpulse: I love the look of the buttons along the top, and I do like having the result window side-by-side when the window size is large. However, I think it would be nicer to have the code on the left and the results on the right. It would be really nice to have the results panel move below the code editor when the window is narrow too. The JSON API exposed by http://playtest.rust-lang.org/ supports CORS, so it can be used without hosting the backend. |
margin-bottom: 20px; | ||
text-decoration: none; | ||
color: #333; | ||
font-family: Helvetica Neue, Helvetica, Arial, sans-serif; |
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.
It would be nice to use a consistent open-source font here as well. If we're using Source Code Pro for monospace, using Source Sans Pro for the sans-serif font would be nice as they're designed to be used together.
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 agree. Helvetica Neue is only available if you bought the font or you're on a mac, so not really good to use.
Thanks for picking this up @thehydroimpulse. I agree with @alan-andrade that I would rather the buttons match those on www, and @thestinger that the page should use different layouts for small screens. |
@brson No worries! Yep, I'm working on some new changes that'll address those. |
@thehydroimpulse @SergioBenitez I pulled rust-lang/prev.rust-lang.org#26 for www today, which adds a live editor to the example snippet. Now we also need to make sure these two editors are consistent. |
FWIW I think the color scheme used for the code on www could be more attractive. |
Sounds good. I'll get the changes done this weekend! |
There's now syntax highlighting for the assembly and LLVM IR output, and I switched to the GitHub theme because it was available for both Ace and Pygments. Since it's the theme anyone looking at the source on GitHub will see, that's a nice form of consistency. |
Would be nice to get the syntax highlighting scheme we have currently into Ace. |
Here's the latest screenshot. Still working on the header/color combos. The current logo is super hard to make it everything look good with. Using the color scheme that @bjz came up with (https://kuler.adobe.com/Rust-color-theme-2709677/) Note: I'm working on the layout, so nothing is properly aligned and there are hidden elements that I haven't styled yet. |
Can we deploy this as it stands and keep improving? |
Yep. I'll get what I have working again and push my changes. |
@brson Just pushed the changes. This can now be landed in it's current state. |
Thanks! I'll leave it to @thestinger to decide. |
It's hard for me to review this because there's so much going on in the same few commits, and parts are being undone in the most recent commit. The JavaScript is going from 137 lines to 572 so there's a lot more than style changes mixed in here. If this was split into much more incremental commits, then I could accept some and we could discuss the ones I have concerns about. |
</div> | ||
</div> | ||
</div> | ||
<script src="//cdnjs.cloudflare.com/ajax/libs/ace/1.1.3/ace.js" charset="utf-8"></script> |
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 don't think these should be moved down here. It's not going to speed up the load time.
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.
That being said, the HTML5 async attribute can work there too.
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.
There's nothing useful on the page before the JavaScript DOMContentLoaded
handler triggers. I don't think the editor is even sized correctly before then.
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.
This is needed for the JavaScript. Otherwise, the code needs to run on the DOMContentLoaded
event.
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.
It's also best practice to put the JavaScript at the bottom, then you don't have to worry about blocking or loading events.
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.
Using DOMContentLoaded
and async
is faster than this, if that's the goal. I don't know why you would consider it to be the best practice.
}); | ||
|
||
|
||
function handleResult(result) { |
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.
This isn't a very robust way of detecting a compilation error. It would need to distinguish between compiler output and output from the program, which is going to require modification of the backend.
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 agree. This was taken from rust-www.
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 agree as well. See https://github.com/rust-lang/rust-www/blob/gh-pages/js/editor.js#L87.
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 don't want to include something that's known to be broken. It doesn't really matter if it's freshly written code or taken from another project. @huonw has a pull request open related to this issue but it's not quite ready to merge. The necessary JSON API should be added before the frontend gains support for it, and it would be better if it wasn't tied to other changes.
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.
This has now been changed to take advantage of the new separate_output.
It looks like this has stalled a bit. What do we need to do to move forward? |
I think we were blocking on a server-side patch for better error detection. Most of this PR's code bloat is because of that. |
@thestinger Would it be easier if I removed all the error handling and just dump the results under the text editor? That'll reduce the code bloat by quite a bit. |
I was just about to make small fine tuning to the look of the playpen because the share button just doesn't stand out which I think might have an impact on the way people engage and spread examples etc. Then I found this PR and it seems to be such a huge step forward. Unfortunately it seems work has stopped. What's the state of this? |
Now that #10 is merged I believe there are no more blockers to continuing here. |
@cburgdorf @brson I'll split the PR up into smaller chunks. That'll get things moving. |
* Refactored the JavaScript to be cleaner and more reusable. * Implemented a simple design. * Added dropdown arrow * Changed ace theme (to be more rustic) * Added Rust logo
* Integrated the awesome error reporting that landed in rust-www. * Cleaner Design. * The extra options are hidden by default. * New "More Options" button that toggles the extra options. * Modified the button styles. * New evaluate button!
Updated to use the new separate_output from #10 |
@thehydroimpulse thanks for persevering! @thestinger How does play.rust-lang.org get updated? |
I updated play.rust-lang.org |
@thehydroimpulse, was the share button accidentally removed? On http://play.rust-lang.org/ I'm also unable to get assembly/IR output. Did I update it incorrectly? |
@alexcrichton I just noticed the share button. I think they were added after my PR started. mmm, the assembly/IR worked locally, I'll check it out and get it fixed. |
What happened here? The new design was so nice, why did this very reverted? |
@sinistersnare: It was based on an older revision of master and lost many improvements. I also don't want the code to be completely rewritten based on subjective style preferences. I don't want a long list of half working hacks like parsing |
@thestinger why couldn't we just use media queries and change the layout based off the width of your window?...then everybody is happy |
It could be done that way, but the current vertical layout really needs the newer layout model. The alternative is fixing the output panel scaling issue with JavaScript - which I don't want to do. |
Yeah I agree. That would be ugly to write plus then you have issues of the output panel infringing on the editor space. If nobody jumps on this in the next few weeks and I make enough headway on my projects I might take a crack at it. There has got to be someone in the community that could whip this up in no time though. |
I would love it if someone submitted a patch to fix the output panel to scale to the bottom of the screen. AFAIK, the only way to do this is via JavaScript or the flexbox model as long as it's not using custom button / select elements with a hardcoded size. It would be great if there was a wide model with the panels side by side but the window really has to be quite wide for that and it needs to make sure each panel has at least 80-100 columns based on the font size. |
CSS3 has the new |
The results panel could use some work. I don't have a linux machine right now to try out all the options.
This is still a WIP. Still things I wish to change.