Bug 21582 - Use CodeMirror for *UserJS & *UserCSS
Summary: Use CodeMirror for *UserJS & *UserCSS
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Tomás Cohen Arazi
URL:
Keywords: release-notes-needed
: 13829 (view as bug list)
Depends on:
Blocks: 22584 23315
  Show dependency treegraph
 
Reported: 2018-10-16 15:30 UTC by Andrew Isherwood
Modified: 2019-07-17 16:01 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This enhancement adds javascript syntax highlighting to aid users when editing the JS system preferences.
Version(s) released in:
19.05.00


Attachments
Bug 21582: Use CodeMirror for *UserJS & *UserCSS (154.67 KB, patch)
2019-04-01 13:44 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21582: Use CodeMirror for *UserJS & *UserCSS (154.79 KB, patch)
2019-04-01 15:55 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21582: Use CodeMirror for *UserJS & *UserCSS (154.77 KB, patch)
2019-04-01 18:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2018-10-16 15:30:37 UTC
CodeMirror is already in use, most recently for the SQL reports editor (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20260)

It would be a substantial UX improvement to add it to the editors for the *UserJS & *UserCSS sysprefs, in additional to providing syntax highlighting, features such as smart indenting and bracket matching would be welcome additions.
Comment 1 Owen Leonard 2018-10-16 16:11:15 UTC
Should this be controlled by the existing UseWYSIWYGinSystemPreferences preference, or a separate one? I'm inclined to say use the existing pref.
Comment 2 Tomás Cohen Arazi 2018-10-16 16:48:40 UTC
(In reply to Andrew Isherwood from comment #0)
> CodeMirror is already in use, most recently for the SQL reports editor
> (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20260)
> 
> It would be a substantial UX improvement to add it to the editors for the
> *UserJS & *UserCSS sysprefs, in additional to providing syntax highlighting,
> features such as smart indenting and bracket matching would be welcome
> additions.

And validation?
Comment 3 Katrin Fischer 2018-10-16 17:34:30 UTC
(In reply to Owen Leonard from comment #1)
> Should this be controlled by the existing UseWYSIWYGinSystemPreferences
> preference, or a separate one? I'm inclined to say use the existing pref.

Hm, feels like a different kind of thing to me. We would still write CSS/HTML code which is not WYSIWYG, so the description seems off. I think I'd still like to not use the WYSIWYG, but like to use syntax highlighting.
Comment 4 Owen Leonard 2018-10-16 17:56:26 UTC
Perhaps since Codemirror doesn't actually write any code for you it there doesn't need to be a preference?
Comment 5 Katrin Fischer 2018-10-16 19:37:59 UTC
That would work for me!
Comment 6 Andrew Isherwood 2018-10-17 10:16:59 UTC
Thinking about this a bit more, it would need additions to the koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/*.pref files to indicate that CodeMirror needs to be attached to the resulting textarea and what format it should be set to, so perhaps something like this in the .pref files:

editor_format: css

This could be tested for in the template iterator that generates the list of preferences, and a semantically meaningless attribute could be added to the textarea element, such as:

data-editor-format="css"

This could then be used to determine which elements need CodeMirror attaching to them and what it's formatting should be set to.
Comment 7 Martin Renvoize 2018-10-17 11:11:09 UTC
Hmmm... perhaps I should resurrect my branch that did exactly this... now, where is that branch... it's been a few years!
Comment 8 Owen Leonard 2019-03-27 13:11:26 UTC
(In reply to Martin Renvoize from comment #7)
> Hmmm... perhaps I should resurrect my branch that did exactly this...

Any luck?
Comment 9 Owen Leonard 2019-03-29 14:48:07 UTC
*** Bug 13829 has been marked as a duplicate of this bug. ***
Comment 10 Pablo AB 2019-03-29 17:08:34 UTC
On bug 13829 (2015) I was suggesting just syntax highlight. I would prefer just syntax highlight now than an overkill full editor on 2023.
The suggestion was not only on *UserJS & *UserCSS but all textareas contain source code:

- Reports: SQL textareas
- All sysprefs containing HTML, CSS and JS (AFAIK there is no JSON/YAML).

Probably a full editor is too much and unnecessary (more risk of compatibility issues...). For major work (with beautify and linting) I would work on a proper text editor.
Comment 11 Tomás Cohen Arazi 2019-03-29 17:33:16 UTC
(In reply to Pablo AB from comment #10)
> On bug 13829 (2015) I was suggesting just syntax highlight. I would prefer
> just syntax highlight now than an overkill full editor on 2023.
> The suggestion was not only on *UserJS & *UserCSS but all textareas contain
> source code:
> 
> - Reports: SQL textareas
> - All sysprefs containing HTML, CSS and JS (AFAIK there is no JSON/YAML).
> 
> Probably a full editor is too much and unnecessary (more risk of
> compatibility issues...). For major work (with beautify and linting) I would
> work on a proper text editor.

CodeMirror is already in the codebase. The only difficulty is hooking the initialization in the right templates... and adding the needed syntaxes support (we only have it for SQL).

I asked for YAML support on bug 22584.
Comment 12 Magnus Enger 2019-03-29 17:46:40 UTC
We already have a wysiwyg editor for sysprefs containing HTML, enabled by UseWYSIWYGinSystemPreferences and added in bug 11584.
Comment 13 Owen Leonard 2019-03-29 18:01:47 UTC
(In reply to Magnus Enger from comment #12)
> We already have a wysiwyg editor for sysprefs containing HTML, enabled by
> UseWYSIWYGinSystemPreferences

My plan is to have CodeMirror handle all textareas which handle any kind of code. If UseWYSIWYGinSystemPreferences is enabled, HTML textareas will be WYSIWYG.

Anyone have an opinion on whether ISBD should be handled as HTML? I think yes.
Comment 14 Pablo AB 2019-03-29 18:36:32 UTC
(In reply to Magnus Enger from comment #12)
> We already have a wysiwyg editor for sysprefs containing HTML, enabled by
> UseWYSIWYGinSystemPreferences

Never think about a WYSIWYG editor. Just syntax highlight would be enough (following KISS principle).
Considering all the CSS that the HTML code will have over I found this syspref useless, and therefore probably should be removed.
Comment 15 Owen Leonard 2019-03-29 18:40:01 UTC
(In reply to Pablo AB from comment #14)

> Never think about a WYSIWYG editor.

I will, because this bug isn't about removing the UseWYSIWYGinSystemPreferences system preference.
Comment 16 Owen Leonard 2019-04-01 13:44:04 UTC Comment hidden (obsolete)
Comment 17 Liz Rea 2019-04-01 15:55:59 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2019-04-01 18:43:18 UTC
Created attachment 87301 [details] [review]
Bug 21582: Use CodeMirror for *UserJS & *UserCSS

This patch adds CodeMirror syntax highlighting by default to JS and CSS
system preferences. HTML preferences will use CodeMirror editors if
UseWYSIWYGinSystemPreferences is disabled.

Three new CodeMirror files are added to support three new syntax
highlighting modes: XML (for HTML), CSS, and JS.

A new option is added to *.pref file configurations for textareas which
are intended for HTML, JS, or CSS: syntax. This option is passed to the
CodeMirror configuration to control syntax highlighting mode.

Textareas without a syntax option specified will not have CodeMirror
enabled.

To test, apply the patch and go to Administration -> System preferences.

Test the behavior of several preferences which use <textarea> as their
input. For example:

- OPACUserJS (JS)
- IntranetUserCSS (CSS)
- OpacHeader (HTML)
- BibtexExportAdditionalFields (no highlighting)

Text entry in each of these should have the correct syntax highlighting
applied to them. All data should be saved correctly.

Test with UseWYSIWYGinSystemPreferences both on and off.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2019-04-01 18:44:22 UTC
Great job, Owen. Works as expected. The only doubt I had is if we should use mime types instead. But not important and works great!
Comment 20 Owen Leonard 2019-04-01 23:54:03 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> The only doubt I had is if we should use
> mime types instead.

This was my approach: Use the simple notation ("css" or "javascript") where it was possible, and the mime type ("text/html") where it was necessary for specificity. I'm fine with using the mime type notation consistently if folks prefer.
Comment 21 Martin Renvoize 2019-04-02 15:18:12 UTC
Awesome job, thanks for picking it up Owen!
Comment 22 Nick Clemens 2019-04-12 12:59:55 UTC
Awesome work all!

Pushed to master for 19.05
Comment 23 Martin Renvoize 2019-04-25 13:17:47 UTC
Enhancement will not be backported to 18.11.x series.