Skip to content

[UI Framework] Add KuiCodeEditor as react-ace replacement/wrapper #14026

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

Merged
merged 18 commits into from
Sep 22, 2017

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Sep 18, 2017

This PR adds the new KuiCodeEditor components, which should be a drop-in replacement for react-ace that we are using as a code editor in several places (e.g. the Markdown editor in the TSVB).

This PR also changes the places where react-ace is used by this component. It basically adds the keyboard accessible mode from #13339 to the react-ace editor. It also passes all props down to react-ace, so that it can be truly used as a replacement with additional accessibility functionality.

Fixes #12901

@timroes timroes added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. review labels Sep 18, 2017
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great!! I had a lot of minor suggestions but overall this is sweet.

What do you think of replacing the editor introduced in #13339 with this, perhaps in another PR? It will simplify the code if we only have one way of creating this code editor.

}
}

configureAce = (ace) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: can we rename this method to aceEditorRef to clarify that we're caching a ref here? Can we move it up to follow the constructor, so that the event handlers are grouped together? And can we rename the parameter to aceEditor for consistency with how this variable is referenced elsewhere within this component?


import { htmlIdGenerator, keyCodes } from '../../../services';

export class KuiCodeEditor extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a propTypes declaration for onBlur, width, and height?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

this.idGenerator = htmlIdGenerator();
}

enableOverlay() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this stopEditing to make it clearer how this method interacts with the other parts of this component, and move it next to startEditing? And can we move this.refs.kuiCodeEditorHint.focus(); from onKeyDownAce into this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the renaming, but we cannot move the focus to it, since otherwise we would also focus the hint, when leaving the texteditor (onBlurAce) which we don't want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, but can we move it down so that it's next to startEditing?

}
};

onHintKeyDown = (ev) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this to onKeyDownHint to keep a consistent pattern with the other event handlers?

</p>
</div>
<AceEditor
{...this.props}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the specific props allowed here? I think we should extract those from props and explicitly pass them in. And then in the root element, we should pass in {...props}:

// line 63
      <div
        className="kuiCodeEditorWrapper"
        style={{ width, height }}
        {...this.props}
      >

This way data-test-subj will get assigned to the root element so we can refer to it within functional tests. We'll also be able to assign things like aria-describedby and other attributes. This is a pattern we're using with other UI components in the framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought about this, but decided against it for the following reasons (that are of course up for discussion):

  • AceEditor has quite some properties, that we would need to extract and pass down.
  • It also passes down all additional props to the ace editor itself. Especially for the mentioned aria attributes it would imho make more sense to apply them to the editor, than the wrapper element (that is never focused).

I totally see the point with the testing data-test-subj that you want most likely to be applied to the wrapper element. We have now several options:

  1. We leave it as it is, and pass all properties to the editor, and just extract the ones we need for the wrapper element (which could also include the data-test-subj).
  2. We pass by default all properties down to the editor, and just extract the ones we need (like width and height) and offer a specific property (e.g. wrapperProps), that you can pass arbitrary properties to the wrapper element.
  3. We explicitly extract and pass all supported properties to AceEditor and all others to the wrapper (thus taking us the possibility to add additional (e.g. ARIA) attributes to the editor).
  4. We explicitly extract properties for the editor, but offer an additional editorProps property, which will be assigned to the editor instead of the wrapper element.

Which way would you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point! It sounds like #1 would be the simplest solution. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading it again after a day, I guess I would prefer 2. at the moment. That way we just pull out what we need, but still give the user a general way of attaching any property wherever she wants (using wrapperProps for the wrapper, or just applying them directly for the editor).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would 2 be also okay with you?

Copy link
Contributor

@cjcenizal cjcenizal Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion on Zoom, we've decided to go with option 1.

mode="less"
theme="github"
width="100%"
setOptions={{ fontSize: '14px' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an onBlur callback here to demonstrate this capability, too?

onBlur={window.alert('onBlur')}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

constructor(props) {
super(props);
this.state = {
hintInactive: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this isHintActive to convey its boolean nature and make its role a little clearer? I generally find it easier to reason about booleans if they're positive instead of negative. I think this is because it avoids double negatives like !isHintInactive.


let element;

beforeEach(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put these tests inside another describe block, and add another test above it:

// You'll need to import this above.
import { requiredProps } from '../../test/required_props';

test('is rendered', () => {
  const component = <KuiCodeEditor />;
  expect(render(component)).toMatchSnapshot();
});

You'll need to run the tests again and commit the generated snapshot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add additional tests like this for width and height?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if we should do snapshot testing here, since that would also test, that react-ace doesn't change what they render. I am not sure, whether it would be good to add snapshots test upon the code generated by a library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for the required_props we would first need to look how we handle the props as discussed in the above comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, whether it would be good to add snapshots test upon the code generated by a library.

I think it's OK to test library code if a component depends upon it. The test failure would be valuable because it will alert us to a change in DOM structure which would have otherwise gone unnoticed. This way, if some functional or other structure-reliant tests fail due to this change, we'll have a head's up and be able to clearly see how the structure changed. If the test failure is just noise and the change is superficial, then it's trivial to update the snapshot.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these changes, @timroes! I responded to the open comment threads.

While testing this in the UI Framework, I also came across a minor bug with the example. It's a little difficult to see in this gif, but if you enter some input into the code editor, and then lose focus by hitting ESC or just clicking out of the editor, then the input you entered disappears. Is this because the AceEditor is a controlled component and it expects the value prop to provide its input? If so, then let's just update the example to supply this prop and update the value it contains via the onChange prop.

code_editor_blur_bug

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two minor things I saw when scanning through the code

isHintActive: true
};
this.idGenerator = htmlIdGenerator();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the constructor, this can just be:

export class KuiCodeEditor extends Component {
  state = {
    isHintActive: false
  };

  idGenerator = htmlIdGenerator();

<div
className={classes}
id={this.idGenerator('codeEditor')}
ref="kuiCodeEditorHint"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use string refs, use callback refs (see https://facebook.github.io/react/docs/refs-and-the-dom.html)

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Had a couple small questions/suggestions, but non-blocking.

}
};

stopEditing() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still move this down so it's after startEditing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

</p>
</div>
<div
id="brace-editor"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know why the rest of the editor doesn't get rendered here? Maybe we can use mount in the test instead of render, then call the html() method on the resulting object, and store that string as the snapshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@timroes timroes requested a review from aphelionz September 22, 2017 11:03
Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm likely seeing KuiCodeEditor as a more "high-level" editor abstraction, but it just seems like just a thin wrapper here. I'd prefer an abstraction that hid ace more, but I won't hold up this PR for it.

theme="github"
width="100%"
value={this.state.value}
onChange={(value) => this.setState({ value })}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -124,7 +124,7 @@ class MarkdownPanelConfig extends Component {
<div className="vis_editor__label">Custom CSS (supports Less)</div>
</div>
<div className="vis_editor__ace-editor">
<AceEditor
<KuiCodeEditor
mode="less"
theme="github"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there use-cases for specifying other themes than github? If not we should pull it into KuiCodeEditor. I'd probably rename mode to language too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets back to the question on what level this abstraction should be. In the long term I would agree with you, and rename those, but for now I would just pass them through, since we are currently passing all properties just to the editor, and I think if we start renaming we should make all properties explicit again (which for this first version we decided against).

@@ -2,7 +2,6 @@ import PropTypes from 'prop-types';
import React, { Component } from 'react';
import SeriesEditor from '../series_editor';
import { IndexPattern } from '../index_pattern';
import AceEditor from 'react-ace';
import 'brace/mode/less';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed? It feels like this should only happen within the KuiCodeEditor abstraction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. If we pull this inside KuiCodeEditor which modes do we import? All; just the ones we need at the moment, and people using this with new languages, will need to change KuiCodeEditor too?

Again: I would like to provide a proper code editor at some point, where this should be cleaned up, but I guess not in this first step. (We apparently also include some modes in webpackShims...)

@@ -0,0 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`KuiCodeEditor is rendered 1`] = `"<div class=\\"kuiCodeEditorWrapper\\"><div class=\\"kuiCodeEditorKeyboardHint\\" id=\\"42\\" tabindex=\\"0\\" role=\\"button\\"><p class=\\"kuiText kuiVerticalRhythmSmall\\">Press Enter to start editing.</p><p class=\\"kuiText kuiVerticalRhythmSmall\\">When you’re done, press Escape to stop editing.</p></div><div id=\\"brace-editor\\" style=\\"width: 500px; height: 500px;\\" class=\\" ace_editor ace-tm testClass1 testClass2\\"><textarea class=\\"ace_text-input\\" wrap=\\"off\\" autocorrect=\\"off\\" autocapitalize=\\"off\\" spellcheck=\\"false\\" style=\\"opacity: 0;\\" tabindex=\\"-1\\"></textarea><div class=\\"ace_gutter\\"><div class=\\"ace_layer ace_gutter-layer ace_folding-enabled\\"></div><div class=\\"ace_gutter-active-line\\"></div></div><div class=\\"ace_scroller\\"><div class=\\"ace_content\\"><div class=\\"ace_layer ace_print-margin-layer\\"><div class=\\"ace_print-margin\\" style=\\"left: 4px; visibility: visible;\\"></div></div><div class=\\"ace_layer ace_marker-layer\\"></div><div class=\\"ace_layer ace_text-layer\\" style=\\"padding: 0px 4px;\\"></div><div class=\\"ace_layer ace_marker-layer\\"></div><div class=\\"ace_layer ace_cursor-layer ace_hidden-cursors\\"><div class=\\"ace_cursor\\"></div></div></div></div><div class=\\"ace_scrollbar ace_scrollbar-v\\" style=\\"display: none; width: 20px;\\"><div class=\\"ace_scrollbar-inner\\" style=\\"width: 20px;\\"></div></div><div class=\\"ace_scrollbar ace_scrollbar-h\\" style=\\"display: none; height: 20px;\\"><div class=\\"ace_scrollbar-inner\\" style=\\"height: 20px;\\"></div></div><div style=\\"height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: hidden;\\"><div style=\\"height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: visible;\\"></div><div style=\\"height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: visible;\\">XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX</div></div></div></div>"`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really bad, and likely "impossible" to review if there are changes to the component.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what's up with XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done as @cjcenizal suggested to render out the whole DOM of ACE, and not just the more or less empty wrapper element. I still have mixed feelings about rendering library code in general and compare it, but I would leave the level that we test this snapshot up to your discussion :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, I didn't realize it wasn't going to be formatted! It's not so bad when it's formatted, but then there are all the escape double-quotes, too... bah...

<div class=\\ "kuiCodeEditorWrapper\\">
    <div class=\\ "kuiCodeEditorKeyboardHint\\" id=\\ "42\\" tabindex=\\ "0\\" role=\\ "button\\">
        <p class=\\ "kuiText kuiVerticalRhythmSmall\\">Press Enter to start editing.</p>
        <p class=\\ "kuiText kuiVerticalRhythmSmall\\">When you’re done, press Escape to stop editing.</p>
    </div>
    <div id=\\ "brace-editor\\" style=\\ "width: 500px; height: 500px;\\" class=\\ " ace_editor ace-tm testClass1 testClass2\\">
        <textarea class=\\ "ace_text-input\\" wrap=\\ "off\\" autocorrect=\\ "off\\" autocapitalize=\\ "off\\" spellcheck=\\ "false\\" style=\\ "opacity: 0;\\" tabindex=\\ "-1\\"></textarea>
        <div class=\\ "ace_gutter\\">
            <div class=\\ "ace_layer ace_gutter-layer ace_folding-enabled\\"></div>
            <div class=\\ "ace_gutter-active-line\\"></div>
        </div>
        <div class=\\ "ace_scroller\\">
            <div class=\\ "ace_content\\">
                <div class=\\ "ace_layer ace_print-margin-layer\\">
                    <div class=\\ "ace_print-margin\\" style=\\ "left: 4px; visibility: visible;\\"></div>
                </div>
                <div class=\\ "ace_layer ace_marker-layer\\"></div>
                <div class=\\ "ace_layer ace_text-layer\\" style=\\ "padding: 0px 4px;\\"></div>
                <div class=\\ "ace_layer ace_marker-layer\\"></div>
                <div class=\\ "ace_layer ace_cursor-layer ace_hidden-cursors\\">
                    <div class=\\ "ace_cursor\\"></div>
                </div>
            </div>
        </div>
        <div class=\\ "ace_scrollbar ace_scrollbar-v\\" style=\\ "display: none; width: 20px;\\">
            <div class=\\ "ace_scrollbar-inner\\" style=\\ "width: 20px;\\"></div>
        </div>
        <div class=\\ "ace_scrollbar ace_scrollbar-h\\" style=\\ "display: none; height: 20px;\\">
            <div class=\\ "ace_scrollbar-inner\\" style=\\ "height: 20px;\\"></div>
        </div>
        <div style=\\ "height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: hidden;\\">
            <div style=\\ "height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: visible;\\"></div>
            <div style=\\ "height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: visible;\\">XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX</div>
        </div>
    </div>
</div>

Copy link
Contributor

@cjcenizal cjcenizal Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbekkelund @timroes Which of these three options do you think makes the most sense? I think I'd prefer 2, but I can accept 1.

1) Use debug()

expect(mount(component).debug()).toMatchSnapshot();

Yields:

<KuiCodeEditor aria-label="aria-label" className="testClass1 testClass2" data-test-subj="test subject string">
  <div className="kuiCodeEditorWrapper" style={{...}}>
    <div className="kuiCodeEditorKeyboardHint" id={42} tabIndex="0" role="button" onClick={[Function]} onKeyDown={[Function]}>
      <p className="kuiText kuiVerticalRhythmSmall">
        Press Enter to start editing.
      </p>
      <p className="kuiText kuiVerticalRhythmSmall">
        When you’re done, press Escape to stop editing.
      </p>
    </div>
    <ReactAce aria-label="aria-label" className="testClass1 testClass2" data-test-subj="test subject string" onBlur={[Function]} name="brace-editor" focus={false} mode="" theme="" height="500px" width="500px" value="" fontSize={12} showGutter={true} onChange={{...}} onPaste={{...}} onLoad={{...}} onScroll={{...}} minLines={{...}} maxLines={{...}} readOnly={false} highlightActiveLine={true} showPrintMargin={true} tabSize={4} cursorStart={1} editorProps={{...}} setOptions={{...}} wrapEnabled={false} enableBasicAutocompletion={false} enableLiveAutocompletion={false}>
      <div id="brace-editor" style={{...}} />
    </ReactAce>
  </div>
</KuiCodeEditor>

2) Use prettified html()

// Note that this requires the `html` dependency, which the UI Framework already uses.
import html from 'html';
expect(html.prettyPrint(mount(component).html(), {
  indent_size: 2,
  unformatted: [], // Expand all tags, including spans
})).toMatchSnapshot();

Yields:

<div class="kuiCodeEditorWrapper">
  <div class="kuiCodeEditorKeyboardHint" id="42" tabindex="0" role="button">
    <p class="kuiText kuiVerticalRhythmSmall">Press Enter to start editing.</p>
    <p class="kuiText kuiVerticalRhythmSmall">When you’re done, press Escape to stop editing.</p>
  </div>
  <div id="brace-editor" style="width: 500px; height: 500px;" class=" ace_editor ace-tm testClass1 testClass2">
    <textarea class="ace_text-input" wrap="off" autocorrect="off" autocapitalize="off"
    spellcheck="false" style="opacity: 0;" tabindex="-1"></textarea>
    <div class="ace_gutter">
      <div class="ace_layer ace_gutter-layer ace_folding-enabled"></div>
      <div class="ace_gutter-active-line"></div>
    </div>
    <div class="ace_scroller">
      <div class="ace_content">
        <div class="ace_layer ace_print-margin-layer">
          <div class="ace_print-margin" style="left: 4px; visibility: visible;"></div>
        </div>
        <div class="ace_layer ace_marker-layer"></div>
        <div class="ace_layer ace_text-layer" style="padding: 0px 4px;"></div>
        <div class="ace_layer ace_marker-layer"></div>
        <div class="ace_layer ace_cursor-layer ace_hidden-cursors">
          <div class="ace_cursor"></div>
        </div>
      </div>
    </div>
    <div class="ace_scrollbar ace_scrollbar-v" style="display: none; width: 20px;">
      <div class="ace_scrollbar-inner" style="width: 20px;"></div>
    </div>
    <div class="ace_scrollbar ace_scrollbar-h" style="display: none; height: 20px;">
      <div class="ace_scrollbar-inner" style="height: 20px;"></div>
    </div>
    <div style="height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: hidden;">
      <div style="height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: visible;"></div>
      <div style="height: auto; width: auto; top: 0px; left: 0px; visibility: hidden; position: absolute; white-space: pre; overflow: visible;">XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX</div>
    </div>
  </div>
</div>

3) Use render(), AKA you're over-thinking it CJ

expect(render(component)).toMatchSnapshot();

Yields:

<div
  class="kuiCodeEditorWrapper"
>
  <div
    class="kuiCodeEditorKeyboardHint"
    id="42"
    role="button"
    tabindex="0"
  >
    <p
      class="kuiText kuiVerticalRhythmSmall"
    >
      Press Enter to start editing.
    </p>
    <p
      class="kuiText kuiVerticalRhythmSmall"
    >
      When you’re done, press Escape to stop editing.
    </p>
  </div>
  <div
    id="brace-editor"
    style="width:500px;height:500px;"
  />
</div>

@kimjoar
Copy link
Contributor

kimjoar commented Sep 22, 2017

Ah, the initial comment specifically says "drop-in replacement" for react-ace, so this works for me. I think long-term a better abstraction would be more specific to our use-cases though. But that can be a separate PR as some point.

@timroes
Copy link
Contributor Author

timroes commented Sep 22, 2017

In the long run I would also like to abstract ACE away more, and provide a maybe also under the hood better code editor. But currently we would "just" like to stick with ACE, but have it accessible. So for now this will just be a thin wrapper, but hopefully evolve to something more in the future. Maybe with K7 :-)

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd love a better abstraction, but this is a good start.

@timroes
Copy link
Contributor Author

timroes commented Sep 22, 2017

Jenkins, test this

@timroes
Copy link
Contributor Author

timroes commented Sep 22, 2017

I created a follow-up issue for a proper KuiCodeEditor: #14119

@timroes timroes merged commit bdc37be into elastic:master Sep 22, 2017
@timroes timroes deleted the kui-code-editor branch September 22, 2017 15:09
timroes added a commit that referenced this pull request Sep 22, 2017
…4026)

* Create KuiCodeEditor component

* Add additional tests

* Add PropTypes for KuiCodeEditor

* Rename hintInactive to isHintActive

* Rename enableOverlay to stopEditing

* Rename and move configureAce method

* Rename onHintKeyDown to onKeyDownHint

* Fix broken configureAce call

* Add onBlur to editor example

* Regroup test cases

* Don't lose value in KuiCodeEditor example

* Remove window.alert, due to annoying behavior when switching tabs

* Remove unnecessary constructor

* Replace string ref by callback ref

* Add a snapshot test

* Move stop editing method

* Use mount to render editor during test

* Extract setState into method in example
@timroes
Copy link
Contributor Author

timroes commented Sep 22, 2017

Backports:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v6.1.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants