User tests: Successful: Unsuccessful:
This PR makes quite a few changes to the CodeMirror plugin (not the javascript library itself).
Labels |
Added:
?
|
Thanks!
The build fails because of PHPCS problems in a layout file. The Joomla! phpcs rules are not suitable for writing layout or template files.
... or you know maybe I could just cut that extra line at the end of the file.
Category | ⇒ | External Library Plugins |
Category | External Library Plugins | ⇒ | Plugins |
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.
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 ..
@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?
@okonomiyaki3000 they are OK for now. They can be reviewed at a later date when we do a general review of all strings.
@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..
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...
@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.
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/
@okonomiyaki3000 Correct.
@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?
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
@brianteeman Yes, that's the one. No problem to add this if it's something people want.
@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.
@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.
@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
Thanks for accepting those fixes.
All tested good from me
@bluezeyes here you go #5874
@test ok
Status | Pending | ⇒ | Ready to Commit |
Thanks for your tests and the PR. Moving this here to RTC
Labels |
Added:
?
|
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!
Milestone |
Added: |
Labels |
Added:
?
|
That's cool. I'd like more time for feedback anyway.
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.
@test ... Works OK
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).
Setting RTC
@okonomiyaki3000 I'm going to merge this into the 3.5 branch are there any changes needed?
@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.
Labels |
Added:
?
|
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.
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.
You know... 63 commits is a little much for this. Maybe I should rework this PR a little...
@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?
@okonomiyaki3000 Github again conflicts Do you plan any more work for this?
Status | Ready to Commit | ⇒ | Pending |
Labels |
Setting this to pending as it requires more work.
Labels |
Removed:
?
|
Milestone |
Removed: |
I'll look at fixing the conflicts. I don't think I had anything more to add at this time.
This PR has received new commits.
CC: @acjunior01, @anibalsanchez, @brianteeman, @dgt41, @yvesh
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.
Milestone |
Added: |
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
With the patch
This PR has received new commits.
CC: @acjunior01, @anibalsanchez, @brianteeman, @dgt41, @yvesh
@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.
@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 ;)
That's not how it works. $modeURL
is really just a url pattern. CodeMirror will use it to find whichever mode it needs.
So, all is OK now and we just need to test it right?
I have tested this item unsuccessfully on 657b77d
@okonomiyaki3000 the colors are still broken for me. It looks like the fix don't work?
I have tested this item 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 :(
I have tested this item successfully on 657b77d
Test OK
Milestone |
Added: |
Milestone |
Removed: |
Status | Pending | ⇒ | Ready to Commit |
Labels |
RTC thanks
Labels |
Added:
?
|
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.
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-11-07 13:14:16 |
Closed_By | ⇒ | wilsonge |
Labels |
Removed:
?
|
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