Add button in playground to run formatter #3652

samueleaton · 2016-12-08T01:57:01Z

On top of just formatting your code, this is also a quicker way for users to check syntax errors.

oprypin · 2016-12-08T06:07:57Z

Good stuff!

Hmm, the implementation seems heavy.

There's quite some copypaste going on in JavaScript.
CSS is done from scratch, but maybe there is a premade style to reuse.

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?

samueleaton · 2016-12-08T14:05:14Z

@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.

oprypin · 2016-12-08T14:17:13Z

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.

No problem, it can be below the editor and to the left of the 'show output' icon. Needs some float action.

I also had a smaller Run Formatter btn but it was more difficult to click on.

Well, the "show output" button is more frequently used and it's small, so I'm not sure about this argument.

I had the text next to the button but it starts to fold to the next line when the window width gets to small.

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

oprypin · 2016-12-08T14:18:20Z

src/compiler/crystal/tools/playground/views/_index.html

@@ -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>


What I had in mind:

<div class="col s7 right-align"> <small class="crystal-version grey-text text-lighten-1 left"></small> <span class="octicon octicon-checklist"></span> </div>

Best part is you don't need that big piece of CSS

Oh yes I like it there too. Although the heavy black is too contrasting from the show output. I'll make a change.

samueleaton · 2016-12-08T15:49:41Z

Well, the "show output" button is more frequently used and it's small, so I'm not sure about this argument.

I think "show output" is too small too.

Sija · 2016-12-08T16:27:36Z

src/compiler/crystal/tools/playground/public/session.js

@@ -578,6 +599,34 @@ Playground.Session = function(options) {
    this._scheduleRun();
  }.bind(this));

+  // code formatter
+  const codeFormatter = (() => {


AFAIK ES2015 is not used anywhere else in the frontend code, so wouldn't this break this pattern and the code for older browsers?

Ahh yes good point. I'm used to transpiling.

@Sija Are developers not using modern browsers though.

@samueleaton maybe, even still it would be inconsistent with rest of the code.

samueleaton · 2016-12-08T16:38:08Z

@BlaXpirit I think something like this is a better UX

UX Reasoning:
(1) Both buttons are larger -> easier to find and click
(2) Buttons are now consistent in styling
(3) Icons allow for people to associate an image with an action so they don't need to read it after they learn the icon
(4) Text in the button allows people who haven't used the button yet know what it does.

oprypin · 2016-12-08T16:42:44Z

@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.

samueleaton · 2016-12-08T17:01:29Z

@BlaXpirit The is the screenshot of the latest commit:

There should be a lot less css I'm adding now.

Better?

Sija · 2016-12-08T17:08:43Z

src/compiler/crystal/tools/playground/public/session.js

@@ -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);


debug leftovers

yeah I caught that right after committing ;)

Sija · 2016-12-08T17:14:09Z

@samueleaton Such heavy coloured buttons taking over too much role of CTA, which should be left to RUN button alone (IMO).

Sija · 2016-12-08T17:15:47Z

src/compiler/crystal/tools/playground/public/session.js

+    iconCont.classList.add('octicon', 'octicon-checklist');
+
+    function run() {
+      if (isRunning)


curly braces plz :)

Sija · 2016-12-08T17:16:10Z

src/compiler/crystal/tools/playground/public/session.js

+    var publicProps = { run, stop };
+
+    Object.defineProperty(publicProps, "isRunning", {
+      get: () => isRunning


still ES2015

samueleaton · 2016-12-08T17:26:09Z

Latest screenshot

bcardiff · 2016-12-08T20:05:29Z

Thanks for jumping to the playground!
Wouldn't be simpler to just run the formatter on every code execution? The server can send as a first response the formatted code. This will result in less JS code and an experience similar to editors. This is how i would do it, but i am happy to discuss.

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.

samueleaton · 2016-12-08T20:46:32Z

@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.

samueleaton · 2016-12-15T15:59:07Z

@bcardiff @asterite If you like this I can squash.

Sija · 2017-01-06T19:22:31Z

@bcardiff @asterite is this PR GTG? would be nice to have in the next release.

bcardiff · 2017-01-06T19:36:42Z

I doubt it will land on 0.20.4
I want do some dog fooding and check that the about page / workbooks still works.
And I'm sure you prefer not to delay the next release :-)

In the last weeks I've been focused on other areas.
I will try to make it next week.

bcardiff

@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!

bcardiff · 2017-01-10T20:26:19Z

src/compiler/crystal/tools/playground/server.cr

+        return
+      end
+
+      send_with_json_builder do |json, io|


Please update to crystal 0.20.4 send_with_json_builder do |json|

samueleaton · 2017-01-30T02:32:26Z

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?

Error in line 5: instantiating 'Crystal::Playground::Agent:Class#new(String, Int32)'

in src/compiler/crystal/tools/playground/agent.cr:8: instantiating 'HTTP::WebSocket:Class#new(URI)'

    @ws = HTTP::WebSocket.new(URI.parse(url))
                          ^~~

in src/http/web_socket.cr:28: instantiating 'HTTP::WebSocket::Protocol:Class#new(URI)'

    new(Protocol.new(uri))
                 ^~~

in src/http/web_socket/protocol.cr:285: instantiating 'new(String, String, (Int32 | Nil), Bool)'

      return new(host, path, uri.port, tls)
             ^~~

in src/http/web_socket/protocol.cr:251: expanding macro

    {% if !flag?(:without_openssl) %}
    ^

in macro 'macro_4583141936' /Users/samueleaton/dev/github/open-source/crystal/src/http/web_socket/protocol.cr:251, line 8:

   1. 
   2.       if tls
   3.         if tls.is_a?(Bool) # true, but we want to get rid of the union
   4.           context = OpenSSL::SSL::Context::Client.new
   5.         else
   6.           context = tls
   7.         end
>  8.         socket = OpenSSL::SSL::Socket::Client.new(socket, context: context, sync_close: true)
   9.       end
  10.     

instantiating 'OpenSSL::SSL::Socket::Client:Class#new(TCPSocket)'
in src/openssl/ssl/socket.cr:4: instantiating 'super(TCPSocket, OpenSSL::SSL::Context::Client, Bool)'

      super(io, context, sync_close)
      ^~~~~

in src/openssl/ssl/socket.cr:85: instantiating 'OpenSSL::BIO:Class#new(TCPSocket)'

    @bio = BIO.new(io)
               ^~~

in src/openssl/bio.cr:18: instantiating 'IO#read(Slice(UInt8))'

      io.read(Slice.new(buffer, len)).to_i
         ^~~~

in src/flate/reader.cr:68: instantiating 'IO#peek()'

        @peek = @io.peek
                    ^~~~

in src/http/content.cr:107: instantiating 'read_chunk_start()'

          read_chunk_start
          ^~~~~~~~~~~~~~~~

in src/http/content.cr:138: instantiating 'read_chunk_end()'

      read_chunk_end
      ^~~~~~~~~~~~~~

in src/http/content.cr:146: instantiating 'IO#skip(Int32)'

      @io.skip(2)
          ^~~~

in src/io.cr:893: instantiating 'read(Slice(UInt8))'

      read_count = read(buffer.to_slice[0, Math.min(bytes_count, 4096)])
                   ^~~~

in src/gzip/reader.cr:107: instance variable '@extra' of Gzip::Header must be Slice(UInt8), not Nil

          Header.new(first_byte, @io)
                 ^~~

Sija · 2017-04-03T10:40:54Z

@samueleaton now, when 0.20.1 landed with GZIP fixes, rebasing will fix above error.

samueleaton · 2017-04-03T14:18:49Z

@Sija Thanks. Setting a reminder to look into this tonight.

Sija · 2017-04-15T01:49:42Z

@samueleaton ping? :)

Sija · 2017-08-30T23:49:25Z

@samueleaton Do you wish to finish this PR? Otherwise I'd be willing to do it.

samueleaton · 2017-09-04T05:28:46Z

@Sija Sorry. Tomorrow is a holiday in the US. I'll do it.

samueleaton · 2017-09-06T03:21:51Z

whoops. need a little help from a someone who has more git skills than me. I tried to sync my branch with crystal's master branch by doing git fetch && git rebase upstream/master and now its saying I am trying to merge 250+ commits. little help? 😅

should be fixed

samueleaton · 2017-09-06T04:15:43Z

GTG?

sdogruyol · 2017-09-06T05:58:01Z

LGTM 👍

Sija · 2017-09-09T10:01:41Z

@RX14 @asterite @bcardiff 🏓

Sija · 2017-09-21T23:45:55Z

👍 to 🚢

bcardiff · 2017-10-03T13:42:43Z

I rebase locally and play a bit with the formatter.
There are some issues regarding when a long running process is running and the formatter is executed in between:

The lines are reported misplaced because of the old running process in the server.
This is easy fixed if a this.runTag++; is added in Playground.Session.format that starts in line 390. So the message of old running agents are ignored.

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.

samueleaton · 2017-10-04T00:57:53Z

Ok. So the fix will be to not allow the formatter while it is running. Thats sound good?

bcardiff · 2017-10-04T02:18:55Z

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.

asterite · 2018-04-27T23:57:28Z

This PR is stalled. If nothing happens in a couple of ways, it will be closed due to inactivity.

samueleaton · 2018-04-30T20:02:48Z

This should now prevent the formatter from running if the compiler is running.

samueleaton · 2018-05-15T22:33:00Z

@bcardiff WDYT?

bcardiff · 2018-06-04T08:57:45Z

Thanks for the patience @samueleaton

oprypin reviewed Dec 8, 2016

View reviewed changes

Sija reviewed Dec 8, 2016

View reviewed changes

bcardiff added kind:feature topic:tools:playground labels Jan 6, 2017

bcardiff self-assigned this Jan 6, 2017

bcardiff requested changes Jan 10, 2017

View reviewed changes

adds formatter tool to playground

1ad120a

samueleaton force-pushed the feat-playground-formatter branch from dd6f3ac to 1ad120a Compare September 6, 2017 04:09

Sija approved these changes Sep 9, 2017

View reviewed changes

RX14 approved these changes Sep 9, 2017

View reviewed changes

Sam Eaton added 2 commits April 30, 2018 10:06

Merge branch 'master' into feat-playground-formatter

5689e63

saves 'isRunning' state for compiler

32bbc86

RX14 approved these changes May 3, 2018

View reviewed changes

wooster0 approved these changes Jun 2, 2018

View reviewed changes

bcardiff merged commit b1bc037 into crystal-lang:master Jun 4, 2018

bcardiff added this to the 0.25.0 milestone Jun 4, 2018

chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018

Add button in playground to run formatter (crystal-lang#3652)

05cd432

Add button in playground to run formatter #3652

Add button in playground to run formatter #3652

Conversation

samueleaton commented Dec 8, 2016

oprypin commented Dec 8, 2016

samueleaton commented Dec 8, 2016

oprypin commented Dec 8, 2016

oprypin Dec 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueleaton commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueleaton commented Dec 8, 2016 • edited

oprypin commented Dec 8, 2016

samueleaton commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueleaton commented Dec 8, 2016

bcardiff commented Dec 8, 2016

samueleaton commented Dec 8, 2016

samueleaton commented Dec 15, 2016

Sija commented Jan 6, 2017

bcardiff commented Jan 6, 2017

bcardiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueleaton commented Jan 30, 2017 • edited

Sija commented Apr 3, 2017

samueleaton commented Apr 3, 2017

Sija commented Apr 15, 2017

Sija commented Aug 30, 2017

samueleaton commented Sep 4, 2017

samueleaton commented Sep 6, 2017 • edited

samueleaton commented Sep 6, 2017

sdogruyol commented Sep 6, 2017

Sija commented Sep 9, 2017

Sija commented Sep 21, 2017

bcardiff commented Oct 3, 2017

samueleaton commented Oct 4, 2017

bcardiff commented Oct 4, 2017

asterite commented Apr 27, 2018

samueleaton commented Apr 30, 2018

samueleaton commented May 15, 2018

bcardiff commented Jun 4, 2018

oprypin Dec 8, 2016 •

edited

samueleaton commented Dec 8, 2016 •

edited

samueleaton commented Jan 30, 2017 •

edited

samueleaton commented Sep 6, 2017 •

edited