[4.0] Fix Codemirror as a custom elements #20684

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.

Sign up for GitHub

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

Jump to bottom

Merged

laoneo merged 9 commits into joomla:4.0-dev from anuragteapot:codemirror

Jun 22, 2018

Contributor

anuragteapot commented Jun 7, 2018 •

edited by joomla-cms-bot

Loading

Pull Request for Issue #20679

Summary of Changes

As @okonomiyaki3000 Suggested to remove the fullscreen message. I removed it.

Testing Instructions

Set default editor to codemirror in global configuration.
Open com_content and start creating an article. See, that there are no errors in web console.
Open the file administrator/components/com_content/forms/article.xml and the text

<field
name="articletext2"
type="editor"
label="COM_CONTENT_FIELD_ARTICLETEXT_LABEL"
filter="JComponentHelper::filterText"
buttons="true"
/>

after the field with the name articletext

4 . Open file
/administrator/components/com_content/tmpl/article/edit.php

and add the text

<?php echo $this->form->getInput('articletext2'); ?>

after the text

<?php echo $this->form->getInput('articletext'); ?>

Expected result

No error in console.

Fix codemirror as a customelements .
Credit goes to @dgrammatiko #20679 (comment)

Actual result

/media/vendor/codemirror/mode/text/html/text/html.js:1 Failed to load resource: the server responded with a status of 404 (Not Found)


          codemirror fix

eafa453

anuragteapot requested a review from brianteeman as a code owner

June 7, 2018 11:49

joomla-cms-bot added Language Change PR-4.0-dev labels

houndci-bot reviewed

View reviewed changes

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              								'Esc': that.closeFullScreen,

              							};

              							editor.addKeyMap(map);

houndci-bot Jun 7, 2018

Expected indentation of 14 spaces but found 7 tabs indent
Unexpected tab character no-tabs

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              								"Ctrl-Q": that.toggleFullScreen,

              								[that.getAttribute('fs-combo')]: that.toggleFullScreen,

              								'Esc': that.closeFullScreen,

              							};

houndci-bot Jun 7, 2018

Expected indentation of 14 spaces but found 7 tabs indent
Unexpected tab character no-tabs

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              							const map = {

              								"Ctrl-Q": that.toggleFullScreen,

              								[that.getAttribute('fs-combo')]: that.toggleFullScreen,

              								'Esc': that.closeFullScreen,

houndci-bot Jun 7, 2018

Expected indentation of 16 spaces but found 8 tabs indent
Unexpected tab character no-tabs
Unnecessarily quoted property 'Esc' found quote-props

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              							const map = {

              								"Ctrl-Q": that.toggleFullScreen,

              								[that.getAttribute('fs-combo')]: that.toggleFullScreen,

houndci-bot Jun 7, 2018

Expected indentation of 16 spaces but found 8 tabs indent
Unexpected tab character no-tabs

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              							}

              							const map = {

              								"Ctrl-Q": that.toggleFullScreen,

houndci-bot Jun 7, 2018

Expected indentation of 16 spaces but found 8 tabs indent
Unexpected tab character no-tabs
Strings must use singlequote quotes

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              			case 'options':

              			if (oldValue && newValue !== oldValue) {

              				this.refresh(this.element);

              			}

houndci-bot Jun 7, 2018

Expected indentation of 8 spaces but found 3 tabs indent
Unexpected tab character no-tabs

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              		switch (attr) {

              			case 'options':

              			if (oldValue && newValue !== oldValue) {

              				this.refresh(this.element);

houndci-bot Jun 7, 2018

Expected indentation of 10 spaces but found 4 tabs indent
Unexpected tab character no-tabs

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              	attributeChangedCallback(attr, oldValue, newValue) {

              		switch (attr) {

              			case 'options':

              			if (oldValue && newValue !== oldValue) {

houndci-bot Jun 7, 2018

Expected indentation of 8 spaces but found 3 tabs indent
Unexpected tab character no-tabs

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              	attributeChangedCallback(attr, oldValue, newValue) {

              		switch (attr) {

              			case 'options':

houndci-bot Jun 7, 2018

Expected indentation of 6 spaces but found 3 tabs indent
Unexpected tab character no-tabs

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
              	set options(value) { this.setAttribute('options', value); }

              	attributeChangedCallback(attr, oldValue, newValue) {

              		switch (attr) {

houndci-bot Jun 7, 2018

Expected indentation of 4 spaces but found 2 tabs indent
Unexpected tab character no-tabs
Expected a default case default-case

brianteeman approved these changes

View reviewed changes

Contributor

brianteeman left a comment

language string approved

laoneo changed the title ~~Fix Codemirror as a custom elements~~ [4.0] Fix Codemirror as a custom elements


          eslit style

fa9621e

houndci-bot reviewed

View reviewed changes

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
                  this.instance.setOption('fullScreen', false);

                }

                makeMarker() {

houndci-bot Jun 7, 2018

Expected 'this' to be used by class method 'makeMarker' class-methods-use-this

build/webcomponents/js/editor-codemirror/editor-codemirror.js

    
                    }

                  } else {

                    while (typeof window[string1] !== 'function') {

                      await this.rafAsync();

houndci-bot Jun 7, 2018

Unexpected await inside a loop no-await-in-loop

build/webcomponents/js/editor-codemirror/editor-codemirror.js

    
                async checkElement(string1, string2) {

                  if (string2) {

                    while (typeof window[string1][string2] !== 'function') {

                      await this.rafAsync();

houndci-bot Jun 7, 2018

Unexpected await inside a loop no-await-in-loop

build/webcomponents/js/editor-codemirror/editor-codemirror.js Outdated

    
                }

                rafAsync() {

                  return new Promise(resolve => {

houndci-bot Jun 7, 2018

Expected parentheses around arrow function argument having a body with curly braces arrow-parens

build/webcomponents/js/editor-codemirror/editor-codemirror.js

    
                  this.instance = window.CodeMirror.fromTextArea(element, this.options);

                }

                rafAsync() {

houndci-bot Jun 7, 2018

Expected 'this' to be used by class method 'rafAsync' class-methods-use-this

ghost mentioned this pull request

[4.0] Codemirror: Fullscreen mode opens the wrong editor and "Failed to load resource" if more than one editor exists #20679

Closed

Anu1601CS added 2 commits

June 7, 2018 18:14


          eslint code style

5bf5f44


          await

c8da110

houndci-bot reviewed

View reviewed changes

build/webcomponents/js/editor-codemirror/editor-codemirror.js

    
                    }

                  } else {

                    while (typeof window[string1] !== 'function') {

                      await this.rafAsync();

houndci-bot Jun 7, 2018

Unexpected await inside a loop no-await-in-loop

build/webcomponents/js/editor-codemirror/editor-codemirror.js

    
                async checkElement(string1, string2) {

                  if (string2) {

                    while (typeof window[string1][string2] !== 'function') {

                      await this.rafAsync();

houndci-bot Jun 7, 2018

Unexpected await inside a loop no-await-in-loop

build/webcomponents/js/editor-codemirror/editor-codemirror.js

    
                  this.instance = window.CodeMirror.fromTextArea(element, this.options);

                }

                rafAsync() {

houndci-bot Jun 7, 2018

Expected 'this' to be used by class method 'rafAsync' class-methods-use-this

Contributor Author

anuragteapot commented Jun 7, 2018 •

edited

Loading

@dgrammatiko Please take a look in the code I have done some changes.

Contributor Author

anuragteapot commented Jun 7, 2018

And, also on eslint codestyle issue . Thanks

Contributor

astridx commented Jun 7, 2018

I have tested this item ✅ successfully on c8da110

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20684.}

Contributor

astridx commented Jun 7, 2018 •

edited

Loading

I saw no error in console and the language string is changed like explained. I would only suggest to change the language string into something like this:
"In read only mode, a full screen is not possible"
Because the full screen does not open in read only mode.

If more than one editor is in write mode (no read only) on the page, the F10 key opens the editor where the cursor is currently located.

Member

laoneo commented Jun 8, 2018

Can you also have a look on the remaining three hound issues?

Anu1601CS and others added 2 commits

June 11, 2018 17:25


          some more hound issues

7471c5e


          Merge branch '4.0-dev' into codemirror

fb68b6a

Contributor Author

anuragteapot commented Jun 11, 2018

Member

laoneo commented Jun 11, 2018

@okonomiyaki3000 can you please test this pr as you are the code mirror expert? Thanks.

Contributor Author

anuragteapot commented Jun 11, 2018 •

edited

Loading

@brianteeman Can you please check language string? Thanks

Contributor

brianteeman commented Jun 11, 2018

I already have :)


          Merge branch '4.0-dev' into codemirror

1bfe982

Contributor

okonomiyaki3000 commented Jun 14, 2018 •

edited

Loading

A few problems still:

The theme selector lists all themes twice. Once normally and once as .min.
After saving the plugin options, the editor is broken. Console says Uncaught SyntaxError: Unexpected token <
When editing a php file, there seems to be something wrong with syntax highlighting and the html part doesn't do things like tag completion. (now that I try it, this might be broken in the J3 version too...)

Contributor

okonomiyaki3000 commented Jun 14, 2018

Ah, and it's a little out of date. The appearance options tab should show a preview of what the editor will look like with the current settings.

Contributor

okonomiyaki3000 commented Jun 14, 2018

Oh and why not just remove that silly fullscreen message? I added it in the first place because I was changing the fullscreen hotkey from ctrl-q (seriously, who thought that was a good idea?) to something configurable but I figured that after a while we could remove it. People must have adjusted to the change by now, right?

Contributor

okonomiyaki3000 commented Jun 14, 2018

You may like to see #20746 for a way to fix my third point above.

Member

laoneo commented Jun 14, 2018

@Anu1601CS can you have a look on the two first issues from @okonomiyaki3000. Thanks.

Contributor Author

anuragteapot commented Jun 14, 2018

@laoneo I will take a look on it.

Contributor Author

anuragteapot commented Jun 16, 2018

A few problems still:

The theme selector lists all themes twice. Once normally and once as .min.
After saving the plugin options, the editor is broken. Console says Uncaught SyntaxError: Unexpected token <
When editing a php file, there seems to be something wrong with syntax highlighting and the HTML part doesn't do things like tag completion. (now that I try it, this might be broken in the J3 version too...)

@okonomiyaki3000 Can you please share the testing instruction?

And, what you want to do in fullscreen message ?

Contributor

okonomiyaki3000 commented Jun 16, 2018

The theme selector is in the plugin settings in the appearance tab. It's just a list of themes.

If you run the editor with ever having saved any options, it works. When you open the plugin settings and then save, all pages with codemirror on them will have errors.

To see the syntax highlighting issue, just open any php file with code mirror.

For the fullscreen message, I just think we don't need it at all. It doesn't make much sense. If we need to put instructions for using CodeMirror there, we should have a lot more than just that one item but why should we? We don't list instructions for other things.

Contributor Author

anuragteapot commented Jun 22, 2018

@okonomiyaki3000 As you suggested #20746 to fix mode, it works fine and syntax highlight is also good. But, one problem is still present, if two codemirror editor is present in a same page, then we are only able to toggle last codemirror to fullscreen ( only one codemirror makes toggle) .

Contributor Author

anuragteapot commented Jun 22, 2018

@okonomiyaki3000 Seems like i found solution of fullscreen.
And, should i remove fullscreen message for both ?

Anu1601CS and others added 2 commits

June 22, 2018 17:37

fix

af85063


          Merge branch '4.0-dev' into codemirror

f663276

Contributor

okonomiyaki3000 commented Jun 22, 2018

I'm not really the one to ask. I never liked the message at all. I'm sure someone must have asked me to put it there but it was so long ago... So, personally, I'd love to see it gone. If nobody else objects...

Contributor

astridx commented Jun 22, 2018

I have tested this item ✅ successfully on f663276

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20684.}

Contributor

astridx commented Jun 22, 2018

I saw no error in console. I saw no full screen message. I like it, that there is no full screen message. I think it is confusing if there is more than one editor on a page. If people see the text "click F10 to open full-screen" more than one time, they will be wondering which editor will be opened.

If more than one editor is in write mode (no read only) on the page, the F10 key opens still the editor where the cursor is currently located. I think this is OK, because so people who know this feature from the past, can still use it.

Contributor

dgrammatiko commented Jun 22, 2018

I have tested this item ✅ successfully on f663276

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20684.}

Contributor Author

anuragteapot commented Jun 22, 2018

Thanks @dgrammatiko

ghost commented Jun 22, 2018

Ready to Commit after two successful tests.

joomla-cms-bot added the RTC label

laoneo merged commit af62faf into joomla:4.0-dev

joomla-cms-bot removed the RTC label

Member

laoneo commented Jun 22, 2018

Nice one, thanks all involved.

laoneo added this to the Joomla 4.0 milestone

anuragteapot deleted the codemirror branch

June 22, 2018 16:07

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change