Experimental CodeMirror editor #1670

outoftime · 2019-03-14T19:28:22Z

Creates an experimental <CodeMirrorEditor> component, which accepts the same (actually currently a subset of) the props as <Editor> but uses CodeMirror rather than ACE as the underlying editor implementation.

This is an initial spike and is not intended to be full-featured or release-ready. The specific goals of this pull request are:

Render an editor of the correct size, with a fairly sensible/familiar look and feel
Keep editor state in sync with underlying current project state

We do get several other things for free/by default with CodeMirror. The full list of features that are required for parity with the ACE implementation can be found in the checklist in #1434.

In this pull request (and until that checklist is complete), the CodeMirror editor is only used in experimental mode (when the experimental query param is present in the URL). In order to avoid bloating the bundle unnecessarily, the <CodeMirrorEditor> component is loaded asynchronously and lazily, in its own chunk, only in experimental mode. This means that the interface takes slightly longer to fully load in experimental mode.

outoftime · 2019-03-14T19:28:44Z

@wylieconlon relevant to your interests? mind taking a look?

wylieconlon · 2019-03-14T20:07:51Z

Yes, I'll take a look as soon as I can. Thanks for putting this out!

…

On Thu, Mar 14, 2019 at 3:28 PM Mat Brown ***@***.***> wrote: @wylieconlon <https://github.com/wylieconlon> relevant to your interests? mind taking a look? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1670 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAora5HaiVwpjIOJLtm_Z644igO647vuks5vWqLsgaJpZM4b1Cxd> .

wylieconlon

Overall I like the direction of this, my biggest concern is whether we can easily test the various effects without relying too heavily on Ace or CodeMirror APIs. My secondary concern is that a class component might have easier-to-follow logic, for example:

  componentDidMount() {
    this._editor = CodeMirror(this.editorContainer.current, {
      mode: 'javascript',
      theme: 'monokai',
      value: this.props.source,
      lineNumbers: true,
      lineWrapping: true,
    });

That being said, it's definitely the smaller concern. I'd just like to see your thoughts on both testing and class components.

wylieconlon · 2019-03-19T10:55:57Z

gulpfile.js

@@ -28,6 +28,7 @@ const srcDir = 'src';
 const distDir = 'dist';
 const stylesheetsDir = path.join(srcDir, 'css');
 const highlightStylesheetsDir = 'node_modules/highlight.js/styles';
+const codemirrorStylesheet = 'node_modules/codemirror/lib/codemirror.css';


There are multiple built-in color schemes for codemirror that we could use as well- the default is pretty low contrast. For light schemes, I prefer Eclipse or Idea, but you can take a look at the built ins: https://codemirror.net/demo/theme.html

I agree that the default scheme isn't amazing, but I actually went through all the themes that ship with the package and didn't find any I thought were overall better than the default (I also liked the IDE-derived themes but there were things I didn't like that I don't remember now; I think they were mostly in the way they themed HTML). This repository of themes looks promising though...

Anyway, seems like something we can continue to ponder between initial experimental-mode release and actually making this the official editor.

The default scheme is specifically less contrasty than these other options, which made it noticeable when switching back and forth. Totally fine with me to update later

wylieconlon · 2019-03-19T10:56:24Z

.eslintrc

@@ -922,6 +926,7 @@
  "plugins": [
    "import",
    "react",
+    "react-hooks",


If you want to start supporting hooks, should that be merged in separately?

So my basic thinking is:

Hooks are good and class components are bad (more on that below)

We should therefore not introduce any new class components, and when we need the features that require either hooks or class components in a new component, we should use hooks

And soon we should probably transition fully to hooks even though the React folks say not to go all crazy rewriting components for hooks just yet

So, with that in mind, this is the first time since hooks became a thing that we have a new component that needs to use hooks. So, this would be the exact time to "start supporting hooks" (I don't see much argument for, say, introducing the eslint plugin when there are no actual hooks to lint)

Since writing this I'm now developing almost entirely in hooks, I think I'll write up some thoughts on that on a higher-up comment.

wylieconlon · 2019-03-19T10:58:39Z

gulpfile.js

@@ -68,6 +69,7 @@ gulp.task('css', () => {
    src([
      path.join(bowerComponents, 'normalize-css/normalize.css'),
      path.join(highlightStylesheetsDir, 'github.css'),
+      codemirrorStylesheet,
      path.join(stylesheetsDir, '**/*.css'),
    ]).


Because there are multiple codemirror stylesheets, I think we want to define them all using something like this:

src(bowerStylesheets.concat(highlightStyleSheets, codemirrorStylesheets, sourceStylesheets))

Are you cool with what is there now? I'm not sure what the key difference is between what I had before and what you wrote in the above comment.

Based on our discussion of adding stylesheets later this is fine

wylieconlon · 2019-03-19T10:58:56Z

package.json

@@ -218,6 +219,7 @@
    "loadjs": "^3.5.4",
    "lodash-es": "^4.17.11",
    "loop-breaker": "^0.1.0",
+    "lru-cache": "^5.1.1",


Used to cache CodeMirror.Doc instances. Basically I want to hold on to the last few, so that the undo buffer is retained if you're switching back and forth between a couple projects, but don't want to build up an indefinitely growing collection of them.

Yes, makes sense

wylieconlon · 2019-03-19T10:59:28Z

src/components/AceEditor.jsx

+import 'brace/mode/html';
+import 'brace/mode/css';
+import 'brace/mode/javascript';
+import 'brace/theme/monokai';


Where are these styles?

I don't actually know, and I'm not sure if this line of code even does anything. In CodeMirror, monokai is a dark theme...

wylieconlon · 2019-03-19T11:40:15Z