Monaco Text Editor #1746
Monaco Text Editor #1746
Conversation
| @@ -30,6 +30,7 @@ | |||
| "@babel/runtime": "^7.7.2", | |||
| "aws-sdk": "^2.680.0", | |||
| "commander": "^2.20.3", | |||
| "deepforge-keras": "github:deepforge-dev/deepforge-keras", | |||
There was a problem hiding this comment.
This should not be included.
There was a problem hiding this comment.
Nice work, @umesh-timalsina! MonacoEditor should probably be renamed to TextEditor since there isn't any need to rename it (it wasn't named based on the library used internally). This should also help minimize code changes and won't require us to rename it again if we move away from Monaco sometime in the future.
| @@ -0,0 +1,228 @@ | |||
| /*globals defines*/ | |||
There was a problem hiding this comment.
typo. This should be define.
| const config = {language: 'yaml'}; | ||
| this.widget = new TextEditorWidget(this.logger, this.$el, config); | ||
| const config = {language: 'yaml', | ||
| modelURI: 'inmemory://env.yml', |
There was a problem hiding this comment.
These types of settings seem like they should probably be the defaults. In the monaco editor, I would just merge the provided options with the defaults. Something like:
config = _.extend({}, DEFAULT_CONFIG, config);| .monaco-editor { | ||
| outline: none; | ||
| padding: 0px; | ||
| text-align: left !important; |
There was a problem hiding this comment.
I would recommend writing these in scss and then generating this file. If you prefer writing the css here, then delete the scss file (though I honestly don't see any good reasons for writing css over scss).
| this._logger = logger.fork('Widget'); | ||
| const modelURI = config.modelURI ? config.modelURI.split(/\./).shift() : 'inmemory://code'; | ||
| const extension = config.modelURI ? config.modelURI.split(/\./).pop() : '.py'; | ||
| this.modelURI = modelURI + chance.string({length: 5}) + extension; |
There was a problem hiding this comment.
I am actually not convinced we need chance (pulling in a new dependency just to generate a short string) as this seems like it could be done pretty easily with just Math.random(). Am I missing something?
| insertSpaces: true | ||
| }); | ||
|
|
||
| const dontDisplayMiniMap = config.dontDisplayMiniMap || false; |
There was a problem hiding this comment.
As a minor comment, it might be easier to understand this if it is displayMinimap rather than the negation of it.
| fontSize: this.displaySettings.fontSize, | ||
| readOnly: this.readOnly, | ||
| minimap: { | ||
| enabled: !dontDisplayMiniMap |
There was a problem hiding this comment.
See my earlier comment about dontDisplayMinimap - this would be easier to understand if it was just displayMinimap rather than !dontDisplayMinimap.
| const self = this; | ||
| return new Promise((resolve, reject) => { | ||
| require(['MonacoVim'], function (MonacoVim) { | ||
| self.keyBindingAdapter = MonacoVim.initVimMode(self.editor, self.$status[0]); |
There was a problem hiding this comment.
It would be good to use consistent capitalization: keybindingAdapter (to be consistent with this.displaySettings.keybindings)
| @@ -14,6 +14,11 @@ config.blob.fsDir = process.env.DEEPFORGE_BLOB_DIR || config.blob.fsDir; | |||
| config.requirejsPaths.deepforge = './src/common'; | |||
| config.requirejsPaths['aws-sdk-min'] = './node_modules/aws-sdk/dist/aws-sdk.min'; | |||
| config.requirejsPaths.ace = './src/visualizers/widgets/TextEditor/lib/ace'; | |||
There was a problem hiding this comment.
Ace should be removed since it is replaced with monaco.
|
I am closing this because this will cause unnecessary changes to all the unrelated panels and reopening from a new branch. |
On resolving #571, #1725. Here's an implementation of
MonacoTextEditor. After merging this, we should implement the language client services to a python language server, offered by MonacoEditor, for typehints, autocomplete etc... among others.Currently, we use the
TextEditor(ace based), in the following widgets: