? ? ? Success

User tests: Successful: Unsuccessful:

avatar okonomiyaki3000
okonomiyaki3000
22 Jan 2015

This PR makes quite a few changes to the CodeMirror plugin (not the javascript library itself).

  • The plugin file is cleaned up a lot by making use of layouts
  • Layouts are used, not just for HTML but for generating js and css code (there are a lot of nice things about this)
  • There is (has always been) an init function which is run once per page for some global CodeMirror setup and and instantiation function which is called once per instance of CM (usually there is only one per page but anyway...). The instantiation function has been simplified down to practically nothing and most its work is now done in the init function.
  • There's a new (optional) toolbar that gives gui access to a few of CodeMirror's features such as full screen mode and auto indent. It's pretty basic at the moment but maybe more capabilities can be added later.
avatar okonomiyaki3000 okonomiyaki3000 - open - 22 Jan 2015
avatar jissues-bot jissues-bot - change - 22 Jan 2015
Labels Added: ?
avatar dgt41
dgt41 - comment - 22 Jan 2015

I get two notices:
Notice: Undefined variable: cols in /plugins/editors/codemirror/codemirror.php on line 259
Notice: Undefined variable: rows in /plugins/editors/codemirror/codemirror.php on line 260

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2015

Thanks!

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2015

The build fails because of PHPCS problems in a layout file. The Joomla! phpcs rules are not suitable for writing layout or template files.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2015

... or you know maybe I could just cut that extra line at the end of the file.

avatar zero-24 zero-24 - change - 22 Jan 2015
Category External Library Plugins
avatar zero-24 zero-24 - change - 22 Jan 2015
Category External Library Plugins Plugins
avatar brianteeman
brianteeman - comment - 22 Jan 2015

Not a huge fan of the wording used in the new language strings but they
follow the style guide for punctuation and capitalisation etc so are ok for
now. Maybe they can be improved in the future.

avatar bluezeyes
bluezeyes - comment - 22 Jan 2015

Maybe is it possible, that the use of CodeWarrior in the Template Manager are based on Global / User editor settings? As at moment (J! 3.3.6) it is not the case ..

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2015

@brianteeman I'm happy to change those strings if anyone has a suggestion.

@bluezeyes CodeWarrior... なつかしい... Uh, I'll need to look into the template manager issue. It should be based on global settings. What else would it be based on?

avatar brianteeman
brianteeman - comment - 22 Jan 2015

@okonomiyaki3000 they are OK for now. They can be reviewed at a later date when we do a general review of all strings.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.
avatar bluezeyes
bluezeyes - comment - 22 Jan 2015

@okonomiyaki3000 Well, it just tried out this CM+Emmet extension: http://extensions.joomla.org/extension/edition/editors/codemirror-plus-emmet-editor
I could select it in the settings, but it wont work in the template manager.. only in custom Html Module
And i think, its hard coded there .. that was the answer of the developer of this extension..

avatar infograf768
infograf768 - comment - 22 Jan 2015

they are OK for now. They can be reviewed at a later date when we do a general review of all strings.

Please make new string perfect now. You complained that many strings are not OK in en-GB, thus creating a lot of language PRs. Changing these new strings later on will —again— force TTs to do a useless work.
A little respect for the work of others is strongly desired...

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2015

@bluezeyes I think I get it. When you install this extension, do you get the option for a new editor? So that, in your user settings, you can select either 'CodeMirror' or 'CodeMirror + Emmet'?

@infograf768 I agree.

avatar brianteeman
brianteeman - comment - 22 Jan 2015

The language strings are OK - they follow the style guides. They are just
written in a slightly different voice to many of the other strings.

As agreed with the PLT a complete review of all the strings for this sort
of stuff should be done after the release of 3.4 as they will effect
translations.

But if you are desperate for them I can rewrite these tonight but I can not
guarantee that when all the strings are reviewed they might not be changed
again.

On 22 January 2015 at 09:15, Elijah Madden notifications@github.com wrote:

@bluezeyes https://github.com/bluezeyes I think I get it. When you
install this extension, do you get the option for a new editor? So that, in
your user settings, you can select either 'CodeMirror' or 'CodeMirror +
Emmet'?

@infograf768 https://github.com/infograf768 I agree.


Reply to this email directly or view it on GitHub
#5863 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar bluezeyes
bluezeyes - comment - 22 Jan 2015

@okonomiyaki3000 Correct.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2015

@bluezeyes Then the reason is that the jform xml file for the template pages does not simply use your configured editor, it uses 'codemirror' explicitly. It must do this because, otherwise, a wysiwyg might be used. This is a weak point of the 'editor' field type. It would be best if users could select their content editor and code editor independent of one another but that is not a possibility right now.

Alternatively, we could just start including Emmet. It would be absolutely trivial to add this functionality. The only possible issue that Emmet is separate distribution from CodeMirror and created by a different developer. Is there any issue with adding it to Joomla? Licensing or anything like that?

avatar brianteeman
brianteeman - comment - 22 Jan 2015

Do you mean https://github.com/emmetio/emmet/blob/master/LICENSE

If so then there is no problem with the licence - I will leave it to others
to comment about including it

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2015

@brianteeman Yes, that's the one. No problem to add this if it's something people want.

avatar bluezeyes
bluezeyes - comment - 22 Jan 2015

@okonomiyaki3000 Well I tried it with vanilla Joomla 3.3.6 Template Isis No changes except installing CM+E . #Edith:# Arrgh ... I should get rest.. overseen your jForm notice
But if the integration of Emmet is possible that would be great. :)

@brianteeman Yes that is the original one.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2015

@bluezeyes Yeah, as it currently works, since CM+E is a different editor from just 'CodeMirror' (as far as Joomla is concerned) and the form on the template editor is explicitly selecting 'CodeMirror' (despite any system or user-level settings), CM+E will never be used there. So, I'll look into adding support for it in the basic installation. I'll put it in a different PR though, in case it's controversial. If it gets in, CM+E will basically be obsolete.

avatar brianteeman
brianteeman - comment - 22 Jan 2015

@okonomiyaki3000 Elijah you have three PR
1. to change color to colour
2. to change On/Off to Show/Hide
3. to change the tooltips to match Show/Hide

Thanks


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.
avatar brianteeman
brianteeman - comment - 22 Jan 2015

Thanks for accepting those fixes.

All tested good from me


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.
avatar brianteeman brianteeman - test_item - 22 Jan 2015 - Tested successfully
avatar okonomiyaki3000
okonomiyaki3000 - comment - 23 Jan 2015

@bluezeyes here you go #5874

avatar anibalsanchez anibalsanchez - test_item - 23 Jan 2015 - Tested successfully
avatar anibalsanchez
anibalsanchez - comment - 23 Jan 2015

@test ok


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.
avatar dgt41
dgt41 - comment - 23 Jan 2015

@test OK

avatar zero-24 zero-24 - alter_testresult - 23 Jan 2015 - dgt41: Tested successfully
avatar zero-24 zero-24 - change - 23 Jan 2015
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 23 Jan 2015

Thanks for your tests and the PR. Moving this here to RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.
avatar brianteeman brianteeman - change - 24 Jan 2015
Labels Added: ?
avatar Kubik-Rubik
Kubik-Rubik - comment - 25 Jan 2015

PR was submitted after the Feature Freeze on January 20th. Since we have new functionalities (e.g. the toolbar) we can add this PR only in the next version 3.5!

Thank you for your contribution @okonomiyaki3000!

avatar Kubik-Rubik Kubik-Rubik - change - 25 Jan 2015
Milestone Added:
avatar Kubik-Rubik Kubik-Rubik - change - 25 Jan 2015
Labels Added: ?
avatar okonomiyaki3000
okonomiyaki3000 - comment - 25 Jan 2015

That's cool. I'd like more time for feedback anyway.

avatar phproberto
phproberto - comment - 2 Feb 2015

This will also require that we review the layout path to be sure it can be overriden properly. But that requires that we review JLayout before so just leaving it as a note here for future review.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 23 Feb 2015

This may need a few changes once #6150 is merged.

avatar acjunior01
acjunior01 - comment - 14 Mar 2015

@test ... Works OK


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.
avatar acjunior01 acjunior01 - test_item - 14 Mar 2015 - Tested successfully
avatar yvesh
yvesh - comment - 14 Mar 2015

Works fine - didn't find any hiccups so far. For exiting full screen an icon would be nice (not everybody thinks on hitting Escape and not every device has a keyboard).


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.
avatar yvesh yvesh - test_item - 14 Mar 2015 - Tested successfully
avatar brianteeman
brianteeman - comment - 14 Mar 2015

Setting RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.
avatar wilsonge
wilsonge - comment - 25 May 2015

@okonomiyaki3000 I'm going to merge this into the 3.5 branch are there any changes needed?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 26 May 2015

@wilsonge Let me get back to you about that soon. It's probably fine but I'd like to double check.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 29 May 2015

@wilsonge My tests indicate that it will merge but I'm starting to have second thoughts about the toolbar. I think the other improvements in this PR are very good but maybe the toolbar is not quite ready for prime time. What would you think if I remove it from this PR and maybe bring it back sometime in the future.

avatar joomla-cms-bot joomla-cms-bot - change - 16 Jun 2015
Labels Added: ?
avatar okonomiyaki3000
okonomiyaki3000 - comment - 16 Jun 2015

So, I think something like a toolbar and other enhancements (like Emmet) should probably not be part of the basic CodeMirror plugin. For that reason, I'm using the Joomla plugin system to allow CodeMirror's functionality to be extended.

I'm using editors_codemirror as the plugin group for this but, if anyone has a better idea, let me know. Once it gets merged, we're basically married to it.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 16 Jun 2015

I guess that means we can take the 'new feature' label off of this PR. The use of plugins is kind of a feature but it's not directly a user-oriented feature. From the user point of view, there's nothing really new here.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 3 Jul 2015

You know... 63 commits is a little much for this. Maybe I should rework this PR a little...

avatar roland-d
roland-d - comment - 3 Oct 2015

@okonomiyaki3000 I am looking at merging this for 3.5, do you have any more work planned for this or is it ready to go?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar zero-24
zero-24 - comment - 2 Nov 2015

@okonomiyaki3000 Github again conflicts :smile: Do you plan any more work for this?

avatar roland-d roland-d - change - 2 Nov 2015
Status Ready to Commit Pending
Labels
avatar roland-d
roland-d - comment - 2 Nov 2015

Setting this to pending as it requires more work.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar joomla-cms-bot joomla-cms-bot - change - 2 Nov 2015
Labels Removed: ?
avatar zero-24 zero-24 - change - 3 Nov 2015
Milestone Removed:
avatar okonomiyaki3000
okonomiyaki3000 - comment - 4 Nov 2015

I'll look at fixing the conflicts. I don't think I had anything more to add at this time.

avatar joomla-cms-bot
joomla-cms-bot - comment - 4 Nov 2015

This PR has received new commits.

CC: @acjunior01, @anibalsanchez, @brianteeman, @dgt41, @yvesh


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 4 Nov 2015

I rebased this because it had become a mess somehow without a whole set of commits being doubled. There's still a commit that adds the toolbar and then a subsequent one that removes it. Finally, there is no toolbar but, instead, CodeMirror can now be extended with additional plugins. I think this is a much better way to go.

avatar zero-24 zero-24 - change - 4 Nov 2015
Milestone Added:
avatar zero-24
zero-24 - comment - 4 Nov 2015

Code Mirror works good here. Just two mirror we lose the code colors and on the text box above code mirror in the template manger see:

Without the patch
screen shot 2015-11-04 at 02 51 46
With the patch
screen shot 2015-11-04 at 02 51 50


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar joomla-cms-bot
joomla-cms-bot - comment - 6 Nov 2015

This PR has received new commits.

CC: @acjunior01, @anibalsanchez, @brianteeman, @dgt41, @yvesh


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 6 Nov 2015

@zero-24 Thanks for catching that. It's a simple fix but it brings up a question. I have put two hidden parameters in the plugin config. One is the path to Codemirror itself, the other is the path format for for mode files. By putting these values in the config, it makes it possible for someone to modify them (they are hidden so you'd have to first write a plugin that modifies the JForm). This would allow you to use a different version or your own custom package of the CodeMirror js and css files.

OK, that's really quite an unusual thing for someone to want to do. Of course, I know that.

Anyway, I need to prepend JUri::root() too whatever the value is so that it finds the media folder. This means that, if you wanted to use an absolute url that points to codemirror files hosted somwhere else, you can't really do it. Well, you can but now you also have to override the layout file. Which is fine, I suppose. If someone really wants to do all this crazy stuff then overriding a layout file isn't too much to ask. Yeah, this should be fine.

avatar zero-24
zero-24 - comment - 6 Nov 2015

@okonomiyaki3000 what do you think about.

$modeURL = $modePath . $extJS;

if (!file_exists($modeURL))
{
$modeURL = JUri::root(true) . '/' . $modePath . $extJS;
}

^^ in the PHP Block of the init lauout

cm.modeURL = <?php echo json_encode($modeURL); ?>;

^^ in the JS part of the layout. So both should work i guess ;)

avatar okonomiyaki3000
okonomiyaki3000 - comment - 6 Nov 2015

That's not how it works. $modeURL is really just a url pattern. CodeMirror will use it to find whichever mode it needs.

avatar roland-d
roland-d - comment - 6 Nov 2015

So, all is OK now and we just need to test it right?

avatar zero-24 zero-24 - test_item - 6 Nov 2015 - Tested unsuccessfully
avatar zero-24
zero-24 - comment - 6 Nov 2015

I have tested this item :red_circle: unsuccessfully on 657b77d

@okonomiyaki3000 the colors are still broken for me. It looks like the fix don't work?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar zero-24 zero-24 - test_item - 6 Nov 2015 - Tested successfully
avatar zero-24
zero-24 - comment - 6 Nov 2015

I have tested this item :white_check_mark: successfully on 657b77d

@okonomiyaki3000 oops i know it is late. Code mirror is ok you just need to wait one or two secconds more. Than the colors are there. Sorry for the false report :(


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar anibalsanchez anibalsanchez - test_item - 7 Nov 2015 - Tested successfully
avatar anibalsanchez
anibalsanchez - comment - 7 Nov 2015

I have tested this item :white_check_mark: successfully on 657b77d

Test OK


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar roland-d roland-d - change - 7 Nov 2015
Milestone Added:
avatar roland-d roland-d - change - 7 Nov 2015
Milestone Removed:
avatar zero-24 zero-24 - change - 7 Nov 2015
Status Pending Ready to Commit
Labels
avatar zero-24
zero-24 - comment - 7 Nov 2015

RTC thanks


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.

avatar joomla-cms-bot joomla-cms-bot - change - 7 Nov 2015
Labels Added: ?
avatar bluezeyes
bluezeyes - comment - 7 Nov 2015

Yeah! ☺

Von meinem Samsung Gerät gesendet.

-------- Ursprüngliche Nachricht --------
Von: zero-24 notifications@github.com
Datum: 07.11.2015 10:54 (GMT+01:00)
An: joomla/joomla-cms joomla-cms@noreply.github.com
Cc: Jörn-Ingo Weigert jiweigert@gmail.com
Betreff: Re: [joomla-cms] Code Mirror plugin improvements (#5863)

RTC thanksThis comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5863.


Reply to this email directly or view it on GitHub.

avatar wilsonge wilsonge - change - 7 Nov 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-11-07 13:14:16
Closed_By wilsonge
avatar wilsonge wilsonge - reference | 2c0c8ed - 7 Nov 15
avatar wilsonge wilsonge - merge - 7 Nov 2015
avatar wilsonge wilsonge - close - 7 Nov 2015
avatar okonomiyaki3000 okonomiyaki3000 - head_ref_deleted - 9 Nov 2015
avatar wilsonge wilsonge - change - 17 Jan 2016
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment