-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add button in playground to run formatter #3652
Add button in playground to run formatter #3652
Conversation
Good stuff! Hmm, the implementation seems heavy. There's quite some copypaste going on in JavaScript. Crystal version doesn't have to go to the bottom. Maybe it's not better to cram the button in without changing the rest, but worth checking out. What about a small icon to the left of the 'show output' one? |
@BlaXpirit I had the text next to the button but it starts to fold to the next line when the window width gets to small. I also had a smaller Run Formatter btn but it was more difficult to click on. I think the btn should be next to the editor because it doesn't affect the output or run the code, it just affects the editor. I'll see about making the JS more DRY. |
No problem, it can be below the editor and to the left of the 'show output' icon. Needs some
Well, the "show output" button is more frequently used and it's small, so I'm not sure about this argument.
If the button is small, it won't fold Keep in mind that it's fine to disagree, I'm just saying what seems nicest to me |
@@ -6,7 +6,7 @@ | |||
|
|||
<div class="row"> | |||
<div class="col s7"> | |||
<small class="crystal-version grey-text text-lighten-1"></small> | |||
<button id="runFormatterBtn" type="button" class="code-formatter-btn" title="Run code formatter">Run Formatter</button> |
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 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.
Oh yes I like it there too. Although the heavy black is too contrasting from the show output. I'll make a change.
I think "show output" is too small too. |
@@ -578,6 +599,34 @@ Playground.Session = function(options) { | |||
this._scheduleRun(); | |||
}.bind(this)); | |||
|
|||
// code formatter | |||
const codeFormatter = (() => { |
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.
AFAIK ES2015 is not used anywhere else in the frontend code, so wouldn't this break this pattern and the code for older browsers?
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.
Ahh yes good point. I'm used to transpiling.
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.
@Sija Are developers not using modern browsers though.
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.
@samueleaton maybe, even still it would be inconsistent with rest of the code.
@BlaXpirit I think something like this is a better UX UX Reasoning: |
@samueleaton In my opinion, the output should never be hidden but that's another topic. I agree that your latest suggestion would still be an improvement. Please just make sure to reuse existing CSS defined by the framework and follow its design instead of inventing your own. |
@BlaXpirit The is the screenshot of the latest commit: There should be a lot less css I'm adding now. Better? |
@@ -604,6 +604,9 @@ Playground.Session = function(options) { | |||
let isRunning = false; | |||
const btn = document.getElementById('runFormatterBtn'); | |||
btn.addEventListener('click', () => run()); | |||
const iconCont = btn.getElementsByClassName('icon')[0]; | |||
console.log('iconCont: ', iconCont); |
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.
debug leftovers
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.
yeah I caught that right after committing ;)
@samueleaton Such heavy coloured buttons taking over too much role of CTA, which should be left to RUN button alone (IMO). |
iconCont.classList.add('octicon', 'octicon-checklist'); | ||
|
||
function run() { | ||
if (isRunning) |
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.
curly braces plz :)
var publicProps = { run, stop }; | ||
|
||
Object.defineProperty(publicProps, "isRunning", { | ||
get: () => isRunning |
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.
still ES2015
Thanks for jumping to the playground! Please note that session.js is also used in workbooks/about page. Formatter can be not supported there for sure. Share gist/download are supported only in main screen. I mention this because possibly you are not aware of it's existence. I agree the console button is to small. But the console is open when there is a first output to show. We can handle that on another issue. Also note that due to sidebar inspector the usages of puts is not as needed as in console experiments. Last but not least, there is also the possibility that instead of buttons we use shortcuts. As in cmd+enter for running. |
@bcardiff I think running the formatter on every execution should be in the settings. Whether or not it runs by default is a different discussion. I think shortcuts and buttons should coexist. Shortcuts being for power users. |
I doubt it will land on 0.20.4 In the last weeks I've been focused on other areas. |
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.
@samueleaton I've reviewed and check some cases.
When more tools come I would say that there needs to be a feedback that a response from the server is been wait. But we can defer that for the next time.
I've request a single change due to updates in crystal 0.20.4.
If you can update that and rebase/squash some commits I will do the merge.
Thanks!
return | ||
end | ||
|
||
send_with_json_builder do |json, io| |
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.
Please update to crystal 0.20.4 send_with_json_builder do |json|
I updated my code to use the latest version as of now (0.20.5+31) and now I get this in the playground. Sorry. I've been really swamped lately and haven't been following the major changes. What do I need to change?
|
@samueleaton now, when |
@Sija Thanks. Setting a reminder to look into this tonight. |
@samueleaton ping? :) |
@samueleaton Do you wish to finish this PR? Otherwise I'd be willing to do it. |
@Sija Sorry. Tomorrow is a holiday in the US. I'll do it. |
should be fixed |
dd6f3ac
to
1ad120a
Compare
GTG? |
LGTM 👍 |
👍 to 🚢 |
I rebase locally and play a bit with the formatter. The lines are reported misplaced because of the old running process in the server. But then there are some subtle issues that needs a little bit more of work. If the running spinner needs to be stopped when the formatter is run probably. Currently the formatter seems to be able to stop de running process, but not the other way around since the formatter should run fast. That is ok (although the feedback of the running spinner need to be fixed for this case) Another alternative would be to allow only one command at a time, and if there is a running process it needs to be stopped. But I am not sure about this. Other future tools might not change the code at all and would be nice for them to run along with a long running process. |
Ok. So the fix will be to not allow the formatter while it is running. Thats sound good? |
That or "abort"/ignore the current running, format the code, and leave the play button without the progress indicator. I would say that while the format command is been wait, the run button could be disabled since the format command should run quickly. |
This PR is stalled. If nothing happens in a couple of ways, it will be closed due to inactivity. |
This should now prevent the formatter from running if the compiler is running. |
@bcardiff WDYT? |
Thanks for the patience @samueleaton |
On top of just formatting your code, this is also a quicker way for users to check syntax errors.