-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[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
Changes from all commits
ab63f84
f5080e1
95196d1
41eb8dc
e3dee7c
eeabf3e
2c43f57
fbc72a7
2053533
1d5a0cc
62ee513
dabc0ba
cbd2256
95152d1
f665185
5a1e86e
048b28c
a7cd817
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
import Select from 'react-select'; | ||
import createSelectHandler from '../lib/create_select_handler'; | ||
|
@@ -11,6 +10,7 @@ import ColorPicker from '../color_picker'; | |
import YesNo from '../yes_no'; | ||
import MarkdownEditor from '../markdown_editor'; | ||
import less from 'less/lib/less-browser'; | ||
import { KuiCodeEditor } from 'ui_framework/components'; | ||
import { htmlIdGenerator } from 'ui_framework/services'; | ||
const lessC = less(window, { env: 'production' }); | ||
|
||
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there use-cases for specifying other There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
width="100%" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import React, { Component } from 'react'; | ||
|
||
import { | ||
KuiCodeEditor | ||
} from '../../../../components'; | ||
|
||
export default class extends Component { | ||
state = { | ||
value: '' | ||
}; | ||
|
||
onChange = (value) => { | ||
this.setState({ value }); | ||
}; | ||
|
||
render() { | ||
return ( | ||
<KuiCodeEditor | ||
mode="less" | ||
theme="github" | ||
width="100%" | ||
value={this.state.value} | ||
onChange={this.onChange} | ||
setOptions={{ fontSize: '14px' }} | ||
onBlur={() => console.log('KuiCodeEditor.onBlur() called')} | ||
/> | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import React from 'react'; | ||
|
||
import { | ||
GuideDemo, | ||
GuidePage, | ||
GuideSection, | ||
GuideSectionTypes, | ||
GuideText, | ||
} from '../../components'; | ||
|
||
import CodeEditor from './code_editor'; | ||
const codeEditorSource = require('!!raw!./code_editor'); | ||
|
||
export default props => ( | ||
<GuidePage title={props.route.name}> | ||
<GuideSection | ||
title="Code Editor" | ||
source={[{ | ||
type: GuideSectionTypes.JS, | ||
code: codeEditorSource, | ||
}]} | ||
> | ||
<GuideText> | ||
<p> | ||
The KuiCodeEditor component is a wrapper around <code>react-ace</code> (which | ||
itself wraps the ACE code editor), that adds an accessible keyboard mode | ||
to it. You should always use this component instead of <code>AceReact</code>. | ||
</p> | ||
<p> | ||
All parameters, that you specify are passed down to the | ||
underlying <code>AceReact</code> component. | ||
</p> | ||
</GuideText> | ||
|
||
<GuideDemo> | ||
<CodeEditor /> | ||
</GuideDemo> | ||
</GuideSection> | ||
</GuidePage> | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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>"`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And what's up with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
.kuiCodeEditorWrapper { | ||
position: relative; | ||
} | ||
|
||
.kuiCodeEditorKeyboardHint { | ||
position: absolute; | ||
top: 0; | ||
bottom: 0; | ||
right: 0; | ||
left: 0; | ||
background: rgba(255, 255, 255, 0.7); | ||
display: flex; | ||
flex-direction: column; | ||
justify-content: center; | ||
align-items: center; | ||
text-align: center; | ||
opacity: 0; | ||
|
||
&:focus { | ||
opacity: 1; | ||
border: 2px solid $globalColorBlue; | ||
z-index: 1000; | ||
} | ||
|
||
&.kuiCodeEditorKeyboardHint-isInactive { | ||
display: none; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
@import "code_editor"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
import React, { Component } from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import classNames from 'classnames'; | ||
import AceEditor from 'react-ace'; | ||
|
||
import { htmlIdGenerator, keyCodes } from '../../../services'; | ||
|
||
export class KuiCodeEditor extends Component { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a propTypes declaration for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
state = { | ||
isHintActive: true | ||
}; | ||
|
||
idGenerator = htmlIdGenerator(); | ||
|
||
aceEditorRef = (aceEditor) => { | ||
if (aceEditor) { | ||
this.aceEditor = aceEditor; | ||
aceEditor.editor.textInput.getElement().tabIndex = -1; | ||
aceEditor.editor.textInput.getElement().addEventListener('keydown', this.onKeydownAce); | ||
} | ||
}; | ||
|
||
onKeydownAce = (ev) => { | ||
if (ev.keyCode === keyCodes.ESCAPE) { | ||
ev.preventDefault(); | ||
ev.stopPropagation(); | ||
this.stopEditing(); | ||
this.editorHint.focus(); | ||
} | ||
} | ||
|
||
onBlurAce = (...args) => { | ||
this.stopEditing(); | ||
if (this.props.onBlur) { | ||
this.props.onBlur(...args); | ||
} | ||
}; | ||
|
||
onKeyDownHint = (ev) => { | ||
if (ev.keyCode === keyCodes.ENTER) { | ||
ev.preventDefault(); | ||
this.startEditing(); | ||
} | ||
}; | ||
|
||
startEditing = () => { | ||
this.setState({ isHintActive: false }); | ||
this.aceEditor.editor.textInput.focus(); | ||
} | ||
|
||
stopEditing() { | ||
this.setState({ isHintActive: true }); | ||
} | ||
|
||
render() { | ||
const { width, height } = this.props; | ||
const classes = classNames('kuiCodeEditorKeyboardHint', { | ||
'kuiCodeEditorKeyboardHint-isInactive': !this.state.isHintActive | ||
}); | ||
return ( | ||
<div | ||
className="kuiCodeEditorWrapper" | ||
style={{ width, height }} | ||
> | ||
<div | ||
className={classes} | ||
id={this.idGenerator('codeEditor')} | ||
ref={(hint) => { this.editorHint = hint; }} | ||
tabIndex="0" | ||
role="button" | ||
onClick={this.startEditing} | ||
onKeyDown={this.onKeyDownHint} | ||
> | ||
<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> | ||
<AceEditor | ||
{...this.props} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 // line 63
<div
className="kuiCodeEditorWrapper"
style={{ width, height }}
{...this.props}
> This way There was a problem hiding this comment. Choose a reason for hiding this commentThe 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):
I totally see the point with the testing
Which way would you prefer? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would 2 be also okay with you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After discussion on Zoom, we've decided to go with option 1. |
||
ref={this.aceEditorRef} | ||
onBlur={this.onBlurAce} | ||
/> | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
KuiCodeEditor.propTypes = { | ||
height: PropTypes.string, | ||
onBlur: PropTypes.func, | ||
width: PropTypes.string, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
import React from 'react'; | ||
import sinon from 'sinon'; | ||
import { mount } from 'enzyme'; | ||
import { KuiCodeEditor } from './code_editor'; | ||
import { keyCodes } from '../../services'; | ||
import { requiredProps } from '../../test/required_props'; | ||
|
||
// Mock the htmlIdGenerator to generate predictable ids for snapshot tests | ||
jest.mock('../../services/accessibility/html_id_generator', () => ({ | ||
htmlIdGenerator: () => { return () => 42; }, | ||
})); | ||
|
||
describe('KuiCodeEditor', () => { | ||
|
||
let element; | ||
|
||
beforeEach(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we put these tests inside another
You'll need to run the tests again and commit the generated snapshot. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add additional tests like this for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
element = mount(<KuiCodeEditor/>); | ||
}); | ||
|
||
test('is rendered', () => { | ||
const component = <KuiCodeEditor {...requiredProps}/>; | ||
expect(mount(component).html()).toMatchSnapshot(); | ||
}); | ||
|
||
describe('hint element', () => { | ||
|
||
test('should exist', () => { | ||
expect(element.find('.kuiCodeEditorKeyboardHint').exists()).toBe(true); | ||
}); | ||
|
||
test('should be tabable', () => { | ||
expect(element.find('.kuiCodeEditorKeyboardHint').prop('tabIndex')).toBe('0'); | ||
}); | ||
|
||
test('should vanish when hit enter on it', () => { | ||
const hint = element.find('.kuiCodeEditorKeyboardHint'); | ||
hint.simulate('keydown', { keyCode: keyCodes.ENTER }); | ||
expect(hint.hasClass('kuiCodeEditorKeyboardHint-isInactive')).toBe(true); | ||
}); | ||
|
||
test('should be enabled after bluring the ui ace box', () => { | ||
const hint = element.find('.kuiCodeEditorKeyboardHint'); | ||
hint.simulate('keydown', { keyCode: keyCodes.ENTER }); | ||
expect(hint.hasClass('kuiCodeEditorKeyboardHint-isInactive')).toBe(true); | ||
element.instance().onBlurAce(); | ||
expect(hint.hasClass('kuiCodeEditorKeyboardHint-isInactive')).toBe(false); | ||
}); | ||
|
||
}); | ||
|
||
describe('interaction', () => { | ||
|
||
test('bluring the ace textbox should call a passed onBlur prop', () => { | ||
const blurSpy = sinon.spy(); | ||
const el = mount(<KuiCodeEditor onBlur={blurSpy}/>); | ||
el.instance().onBlurAce(); | ||
expect(blurSpy.called).toBe(true); | ||
}); | ||
|
||
test('pressing escape in ace textbox will enable overlay', () => { | ||
element.instance().onKeydownAce({ | ||
preventDefault: () => {}, | ||
stopPropagation: () => {}, | ||
keyCode: keyCodes.ESCAPE | ||
}); | ||
expect(element.find('.kuiCodeEditorKeyboardHint').matchesElement(document.activeElement)).toBe(true); | ||
}); | ||
|
||
}); | ||
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { KuiCodeEditor } from './code_editor'; |
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.
Is this still needed? It feels like this should only happen within the
KuiCodeEditor
abstractionThere 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.
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 changeKuiCodeEditor
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...)