-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove Ivy Codemirror #14659
Remove Ivy Codemirror #14659
Changes from 13 commits
d16a374
0d1126e
fd8ae4a
2364396
f5e4935
0a40a76
4d4591d
14b2f90
c582582
1a7c47b
0de2185
31a2565
e2364fd
d36665f
14893c3
91139d8
549c3b4
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 |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:improvement | ||
ui: Replaces the IvyCodemirror wrapper with a custom ember modifier. | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,50 +14,24 @@ import { action } from '@ember/object'; | |
* @param {Function} [valueUpdated] - action to preform when you edit the codemirror value. | ||
* @param {Function} [onFocusOut] - action to preform when you focus out of codemirror. | ||
* @param {string} [helpText] - helper text. | ||
* @param {object} [options] - option object that overrides codemirror default options such as the styling. | ||
* @param {Object} [extraKeys] - to provide keyboard shortcut methods for things like saving on shift + enter. | ||
* @param {Array} [gutters] - An array of CSS class names or class name / CSS string pairs, each of which defines a width (and optionally a background), and which will be used to draw the background of the gutters. | ||
* @param {string} [mode] - right now we only import ruby so must mode but be ruby or defaults to javascript. If you wanted another language you need to import it into the modifier. | ||
* @param {Boolean} [readOnly] - defaults to false. | ||
* @param {String} [theme] - specify or customize the look via css. | ||
* @param {String} [value] - value within the display. | ||
* @param {String} [viewportMargin] - Size of viewport. Often set to "Infinity" to load/show all text regardless of length. | ||
*/ | ||
|
||
const JSON_EDITOR_DEFAULTS = { | ||
// IMPORTANT: `gutters` must come before `lint` since the presence of | ||
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. moved comment to the modifier. |
||
// `gutters` is cached internally when `lint` is toggled | ||
gutters: ['CodeMirror-lint-markers'], | ||
tabSize: 2, | ||
mode: 'application/json', | ||
lineNumbers: true, | ||
lint: { lintOnChange: false }, | ||
theme: 'hashi', | ||
readOnly: false, | ||
showCursorWhenSelecting: true, | ||
}; | ||
|
||
export default class JsonEditorComponent extends Component { | ||
value = null; | ||
valueUpdated = null; | ||
onFocusOut = null; | ||
readOnly = false; | ||
options = null; | ||
|
||
constructor() { | ||
super(...arguments); | ||
this.options = { ...JSON_EDITOR_DEFAULTS, ...this.args.options }; | ||
if (this.options.autoHeight) { | ||
this.options.viewportMargin = Infinity; | ||
delete this.options.autoHeight; | ||
} | ||
if (this.options.readOnly) { | ||
this.options.readOnly = 'nocursor'; | ||
this.options.lineNumbers = false; | ||
delete this.options.gutters; | ||
} | ||
} | ||
|
||
get getShowToolbar() { | ||
return this.args.showToolbar === false ? false : true; | ||
} | ||
|
||
@action | ||
updateValue(...args) { | ||
if (this.args.valueUpdated) { | ||
onUpdate(...args) { | ||
if (!this.args.readOnly) { | ||
// catching a situation in which the user is not readOnly and has not provided a valueUpdated function to the instance | ||
this.args.valueUpdated(...args); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import { action } from '@ember/object'; | ||
import { bind } from '@ember/runloop'; | ||
import codemirror from 'codemirror'; | ||
import Modifier from 'ember-modifier'; | ||
|
||
import 'codemirror/addon/edit/matchbrackets'; | ||
import 'codemirror/addon/selection/active-line'; | ||
import 'codemirror/addon/lint/lint.js'; | ||
import 'codemirror/addon/lint/json-lint.js'; | ||
// right now we only use the ruby and javascript, if you use another mode you'll need to import it. | ||
// https://codemirror.net/mode/ | ||
import 'codemirror/mode/ruby/ruby'; | ||
import 'codemirror/mode/javascript/javascript'; | ||
|
||
export default class CodeMirrorModifier extends Modifier { | ||
didInstall() { | ||
this._setup(); | ||
} | ||
|
||
didUpdateArguments() { | ||
this._editor.setOption('readOnly', this.args.named.readOnly); | ||
if (!this.args.named.content) { | ||
return; | ||
} | ||
if (this._editor.getValue() !== this.args.named.content) { | ||
this._editor.setValue(this.args.named.content); | ||
} | ||
} | ||
|
||
@action | ||
_onChange(editor) { | ||
this.args.named.onUpdate(editor.getValue(), this._editor); | ||
} | ||
|
||
@action | ||
_onFocus(editor) { | ||
this.args.named.onFocus(editor.getValue()); | ||
} | ||
|
||
_setup() { | ||
if (!this.element) { | ||
throw new Error('CodeMirror modifier has no element'); | ||
} | ||
const editor = codemirror(this.element, { | ||
// IMPORTANT: `gutters` must come before `lint` since the presence of | ||
// `gutters` is cached internally when `lint` is toggled | ||
gutters: this.args.named.gutters || ['CodeMirror-lint-markers'], | ||
matchBrackets: true, | ||
lint: { lintOnChange: true }, | ||
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. The previous version of 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. Good catch. This was left over from me playing with the linting. I'll change. 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. Okay, strange. I changed back and it broke the linter. Let me investigate a little tomorrow because we are calling performLint() so we shouldn't need this. |
||
showCursorWhenSelecting: true, | ||
styleActiveLine: true, | ||
tabSize: 2, | ||
// all values we can pass into the JsonEditor | ||
extraKeys: this.args.named.extraKeys || '', | ||
lineNumbers: this.args.named.lineNumbers, | ||
mode: this.args.named.mode || 'application/json', | ||
readOnly: this.args.named.readOnly || false, | ||
theme: this.args.named.theme || 'hashi', | ||
value: this.args.named.content || '', | ||
viewportMargin: this.args.named.viewportMargin || '', | ||
}); | ||
|
||
editor.on('change', bind(this, this._onChange)); | ||
editor.on('focus', bind(this, this._onFocus)); | ||
|
||
this._editor = editor; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,7 @@ $gutter-grey: #2a2f36; | |
.cm-s-hashi { | ||
&.CodeMirror { | ||
background-color: $black !important; | ||
resize: vertical; | ||
resize: vertical; | ||
color: #cfd2d1 !important; | ||
border: none; | ||
font-family: $family-monospace; | ||
|
@@ -168,10 +168,14 @@ $gutter-grey: #2a2f36; | |
} | ||
|
||
.readonly-codemirror { | ||
.CodeMirror-cursors { | ||
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 not working. We have it working now under a new class that only shows if the editor is readOnly. |
||
.CodeMirror-code { | ||
cursor: default; | ||
} | ||
.CodeMirror-cursor { | ||
// https://github.com/codemirror/CodeMirror/issues/1099 | ||
display: none; | ||
} | ||
|
||
// css for a specific theme | ||
.cm-s-hashi { | ||
span { | ||
color: $light-grey; | ||
|
@@ -195,7 +199,6 @@ $gutter-grey: #2a2f36; | |
} | ||
} | ||
} | ||
|
||
.cm-s-auto-height.CodeMirror { | ||
height: auto; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,10 @@ | |
<JsonEditor | ||
@showToolbar={{false}} | ||
@value={{stringify this.content}} | ||
@options={{hash readOnly=true lineNumbers=false autoHeight=true gutters=false theme="hashi auto-height"}} | ||
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. all options objects were spread out so that the individual properties were sent to the json-editor. 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'm not seeing 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. Good catch. I removed this because before it was used to set the viewportMargin. See here 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. Ahh ok I see now thanks! |
||
@readOnly={{true}} | ||
@viewportMargin="Infinity" | ||
@gutters={{false}} | ||
@theme="hashi auto-height" | ||
/> | ||
<HoverCopyButton @copyValue={{stringify this.content}} /> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,10 +26,6 @@ const appConfig = { | |
return `${config.rootURL.replace(/\/$/, '')}${filePath}`; | ||
}, | ||
}, | ||
codemirror: { | ||
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. everything removed from this file was added into the modifier. |
||
modes: ['javascript', 'ruby'], | ||
keyMaps: ['sublime'], | ||
}, | ||
babel: { | ||
plugins: ['@babel/plugin-proposal-object-rest-spread', ['inline-json-import', {}]], | ||
}, | ||
|
@@ -74,8 +70,7 @@ module.exports = function (defaults) { | |
|
||
app.import('node_modules/jsonlint/lib/jsonlint.js'); | ||
app.import('node_modules/codemirror/addon/lint/lint.css'); | ||
app.import('node_modules/codemirror/addon/lint/lint.js'); | ||
app.import('node_modules/codemirror/addon/lint/json-lint.js'); | ||
app.import('node_modules/codemirror/lib/codemirror.css'); | ||
app.import('node_modules/text-encoder-lite/text-encoder-lite.js'); | ||
app.import('node_modules/jsondiffpatch/dist/jsondiffpatch.umd.js'); | ||
app.import('node_modules/jsondiffpatch/dist/formatters-styles/html.css'); | ||
|
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.
Small nit on the comment wording. Also, from reading it I'm wondering since ruby and application/json are the only supported modes at this time could the arg change to
useRubyMode
or something like that? If true themode
is set toruby
otherwiseapplication/json
is used. What are your thoughts?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.
That's a valid question. I think however I prefer mode because it aligns with the codemirror property. It would be easy for someone to look up. I also don't want someone to think they only can use Ruby in the future. They can use lots of other options they just need to import it. They'll immediately error to if they use something other than ruby.
I'll amend the comment wording.
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.
Sounds good so if I were to pass in
php
right now for examplecodemirror
would throw an error since it's not imported?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.
correct, it wouldn't work.