Swap CodeMirror in CssEditor for monaco-editor #4133

david-poindexter · 2020-09-27T17:42:37Z

Summary

Resolves #4132

Thanks to @valadas for his great help on this PR - it was a lot of fun working on this one!

Light Theme

Dark Theme

Find & Replace

Color Picker

Command Palette

mitchelsellers

Two questions before I approve, this looks awesome by the way!

It looks like an XCOPY inclusion was done here, which most likely makes sense due to our project structure, but wanted to be sure.
If my previous comment is correct, should we strip language support for things that will not be possible, such as PHP, etc.?

david-poindexter · 2020-09-27T18:35:22Z

As mentioned in Swap CodeMirror in CssEditor for monaco-editor #4132 this is a stepping stone, using the current pattern. Since the CssEditor module is not a proper *.Web project with package management, the monaco-editor is placed in the shared components.
Currently, we are controlling the editor language support in the config for the implementation.

valadas · 2020-09-27T18:35:25Z

In my opinion this (which is awesome by the way) is a first implementation but ideally we would rework this and others together to make it use it as a node_module like the rest of the projects. But this feels like a nice stepping stone. We can use this too in sql console and config editor. When we do move it to a node_module then webpack will just grab the languages that are consumed (css, xml, sql)... As saving does not allow actually saving a php file, I see no concern here.

valadas · 2020-09-27T18:35:45Z

I will review this in more details by the way a bit later today...

mitchelsellers · 2020-09-27T18:42:29Z

As saving does not allow actually saving a php file, I see no concern here.

My only concern is files with PHP in them can show up in security scans, even if they do nothing. We already have some issues like that with the CKEditor.

All good here for now.

valadas

Looks awesome to me!

SkyeHoefling · 2020-09-28T14:32:54Z

👋 Hey everyone, I have quite a bit of experience using monaco in DNN. Last year I built my own IDE built into DNN as a proof of concept. Would love to take a look at this before merging. Hoping I can take a look today or tomorrow.

SkyeHoefling

@david-poindexter this will be a great addition to DNN, adding the core VSCode editor the Monaco Editor will be amazing!

I have a few concerns which I highlighted in my comments and will summarize here

The Monaco Editor is pulled into the DNN source code. We really should use a npm package reference as it will be easier to update in the future. VSCode iterates at an incredibly fast pace and if we need to update the version as this PR exists, it will be a time consuming process. Unless there is a build script or some form of automation
This implementation uses the getting started guide's recommendations of RequireJS. Since the DNN Persona Bar core modules all use ReactJS I think it would make sense to use the Monaco Editor's ReactJS component. This will make it easy to drop onto the page and integrate in other core modules.

Is there plans to allow people to switch from dark to light theme or vice-versa? It may be as simple as placing a tooltip explaining how to use the command palette to adjust theme.

Useful NPM Links

Monaco Editor
ReactJS Monaco Component - This is where things get tricky, there are several different versions out there that all have their pros/cons
- react-monaco-editor
- @Monaco-Editor/React - not an official one even though it has @monaco-editor in name. A good implementation that hides a lot of the webpack config

I have used both of the react components that wrap the Monaco Editor in the past. If I recall correctly I landed on the react-monaco-editor. Even though it required more webpack configuration it gave me more control.

SkyeHoefling · 2020-09-28T15:02:51Z

...dminExperience/Dnn.PersonaBar.Extensions/admin/personaBar/Dnn.CssEditor/scripts/CssEditor.js

+        var initCssEditor = function() {
+            var monacoEditorLoaderScript = document.createElement('script');
+            monacoEditorLoaderScript.type = 'text/javascript';
+            monacoEditorLoaderScript.src = '/Resources/Shared/components/MonacoEditor/loader.js';


This PR is maintaining the MonocoEditor minified source code under our shared components. This will make it difficult to maintain the version of the editor DNN is using. Did you try using any of the npm packages that exist for MonacoEditor?

The CSS Editor extension for the Persona Bar does not use React and doesn't have a Webpack config at this point, so this approach was used as a first implementation. The plan is to switch it to use a version from npm.

@david-poindexter @valadas @mitchelsellers Do we want to avoid adding these scripts to the Shared Resources folder if we're planning to remove them soon? We don't want to give developers the impression that they're intended for broader use, right?

@bdukes Maybe for this version it would make sense to include under the /Dnn.CssEditor extension until we include this for usage in other locations and properly pull it in? @david-poindexter thoughts?

Thanks for the clarification on the CSS Editor, I thought something was odd when I didn't see any React code.

Is there an item already on the backlog for updating this to use an NPM package instead of adding the minified source code? Was there a barrier to getting it working?

@ahoefling please refer to #4132 - this is a "stepping stone" solution. This is obviously not the "right way" to do this, but the current CssEditor module in the PersonaBar is not a proper solution with package management. Therefore, that is a task for a future PR.

SkyeHoefling · 2020-09-28T15:08:19Z

...dminExperience/Dnn.PersonaBar.Extensions/admin/personaBar/Dnn.CssEditor/scripts/CssEditor.js

+            monacoEditorLoaderScript.src = '/Resources/Shared/components/MonacoEditor/loader.js';
+            document.body.appendChild(monacoEditorLoaderScript);
+
+            require.config({ paths: { 'vs': '/Resources/Shared/components/MonacoEditor' }});


Using requireJS is a supported code path for MonacoEditor and it is a great way to get started. The DNN PersonaBar core modules use ReactJS and the MonacoEditor has a ReactJS component that is easy to use in that context. If we are adding this control to a ReactJS page I would consider using that component.

I am not 100% sure how this particular module uses the ReactJS components that is the DNN persona bar modules. In the context of ReactJS I find it much easier to configure the editor and manage it than calling the JS apis. That is my preference and I defer to the maintainers opinions on this one.

This Persona Bar extension (CSS Editor) does not use React. I'm not sure if the other extensions which might use Monaco use React or not (SQL and Config Manager).

@ahoefling this module currently is not using ReactJS. It is plain HTML, JS, CSS. Therefore, this is a stepping stone solution as stated in #4132.

Furthermore, I would love that we don't make it React only :) But agree we should pull it as an npm package, which is a future step after this stepping stone.

SkyeHoefling · 2020-09-28T15:08:46Z

...dminExperience/Dnn.PersonaBar.Extensions/admin/personaBar/Dnn.CssEditor/scripts/CssEditor.js

+            var monacoEditor = cssEditor.create(document.getElementById("monaco-editor"), {
+                model: cssContent,
+                language: "css",
+                wordWrap: 'wordWrapColumn',
+                wordWrapColumn: 80,
+                wordWrapMinified: true,
+                wrappingIndent: "indent",
+                lineNumbers: "on",
+                roundedSelection: false,
+                scrollBeyondLastLine: false,
+                readOnly: false,
+                theme: theme,
+                automaticLayout: true
+            });


See comment about about ReactJS component.

See #4132. This is a stepping stone solution. Moving this to a proper module with package management will a future task.

bdukes · 2020-09-28T15:42:31Z

...dminExperience/Dnn.PersonaBar.Extensions/admin/personaBar/Dnn.CssEditor/scripts/CssEditor.js

+            var theme = "vs-light";
+            if (window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) {
+                theme = "vs-dark";
+            }
+            window.matchMedia('(prefers-color-scheme: dark)').addEventListener('change', e => {
+                theme = e.matches ? "vs-dark" : "vs-light";
+            });


@ahoefling regarding your question about switching themes, it looks like the current support is making use of the OS dark/light theme preference.

If it follows the Operating System theme that is probably good enough for now

Correct @bdukes - it detects the user's preference and themes monaco-editor accordingly.

SkyeHoefling · 2020-09-28T15:52:26Z

@bdukes shared some insight about the lack of ReactJS in the CSS Editor module. We can disregard all my comments on the ReactJS npm module from my review.

david-poindexter · 2020-09-28T16:22:59Z

@bdukes shared some insight about the lack of ReactJS in the CSS Editor module. We can disregard all my comments on the ReactJS npm module from my review.

Yes, this will be a future task, and we will have the same issue with the ConfigConsole and SqlConsole modules.

Swap CodeMirror in CssEditor for monaco-editor

5cb9e4e

david-poindexter added the Type: Feature label Sep 27, 2020

david-poindexter added this to the 9.8.0 milestone Sep 27, 2020

david-poindexter self-assigned this Sep 27, 2020

Add license

f1c9887

david-poindexter requested review from mitchelsellers and valadas September 27, 2020 17:57

mitchelsellers reviewed Sep 27, 2020

View reviewed changes

mitchelsellers approved these changes Sep 27, 2020

View reviewed changes

david-poindexter added the Status: On Hold label Sep 27, 2020

valadas approved these changes Sep 27, 2020

View reviewed changes

bdukes approved these changes Sep 28, 2020

View reviewed changes

SkyeHoefling reviewed Sep 28, 2020

View reviewed changes

bdukes reviewed Sep 28, 2020

View reviewed changes

bdukes removed the Status: On Hold label Oct 6, 2020

mitchelsellers merged commit f25f7fe into dnnsoftware:develop Oct 6, 2020

david-poindexter deleted the monaco-css-editor branch October 6, 2020 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap CodeMirror in CssEditor for monaco-editor #4133

Swap CodeMirror in CssEditor for monaco-editor #4133

david-poindexter commented Sep 27, 2020

mitchelsellers left a comment

david-poindexter commented Sep 27, 2020

valadas commented Sep 27, 2020

valadas commented Sep 27, 2020

mitchelsellers commented Sep 27, 2020

valadas left a comment

SkyeHoefling commented Sep 28, 2020

SkyeHoefling left a comment

SkyeHoefling Sep 28, 2020

bdukes Sep 28, 2020

mitchelsellers Sep 28, 2020

SkyeHoefling Sep 28, 2020

david-poindexter Sep 28, 2020

SkyeHoefling Sep 28, 2020

bdukes Sep 28, 2020

david-poindexter Sep 28, 2020

valadas Sep 28, 2020

SkyeHoefling Sep 28, 2020

david-poindexter Sep 28, 2020

bdukes Sep 28, 2020

SkyeHoefling Sep 28, 2020

david-poindexter Sep 28, 2020

SkyeHoefling commented Sep 28, 2020

david-poindexter commented Sep 28, 2020

Swap CodeMirror in CssEditor for monaco-editor #4133

Swap CodeMirror in CssEditor for monaco-editor #4133

Conversation

david-poindexter commented Sep 27, 2020

Summary

Light Theme

Dark Theme

Find & Replace

Color Picker

Command Palette

mitchelsellers left a comment

Choose a reason for hiding this comment

david-poindexter commented Sep 27, 2020

valadas commented Sep 27, 2020

valadas commented Sep 27, 2020

mitchelsellers commented Sep 27, 2020

valadas left a comment

Choose a reason for hiding this comment

SkyeHoefling commented Sep 28, 2020

SkyeHoefling left a comment

Choose a reason for hiding this comment

Useful NPM Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkyeHoefling commented Sep 28, 2020

david-poindexter commented Sep 28, 2020