#11480 closed New Feature (fixed)
Code snippet plugin
Reported by: | Olek Nowodziński | Owned by: | Olek Nowodziński |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.4.0 |
Component: | General | Version: | 4.4.0 |
Keywords: | Cc: |
Description
A brand-new widgets-based plugin to make insertion of rich code snippets into editor contents nice and easy.
Key features:
- Syntax highlighting.
- Server-side?
- A generic architecture working with different server-side libraries (e.g. configurable URL and POST data).
- Geshi as default?
- Note: Server-side highlighting requires AJAX POST (see
CKEDITOR.ajax.post
in linked mockup).
- Client-side syntax highlighting support (unlikely)?
- Server-side?
- Dialog-based editing.
- Mostly due to lack of focusmanager for widgets. Widgets lose focus if
<select>
is clicked.
- Mostly due to lack of focusmanager for widgets. Widgets lose focus if
- Support for different programming languages.
- A select field to switch between languages.
- Downcast to
<pre><code [class="language-*"]>...</code></pre>
- Simple and fast testing
- The architecture of the plugin should be independent and possible to test without any coloring library.
- Pretty sample :)
- KISS. Code-size matters.
Existing mockup created during DrupalCon Prague 2013 sprint.
Attachments (1)
Change History (28)
comment:1 Changed 11 years ago by
Status: | new → confirmed |
---|---|
Summary: | Snippet plugin → Code snippet plugin |
comment:2 Changed 11 years ago by
Owner: | set to Marek Lewandowski |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 11 years ago by
Status: | assigned → review |
---|
comment:4 Changed 11 years ago by
@todo: Since code snippets plugin already has its ticket, its branch should be renamed to t/11480
comment:5 Changed 11 years ago by
comment:7 follow-up: 8 Changed 11 years ago by
Status: | review → review_failed |
---|
WOW. This is a really cool plugin :) As soon as it will be released we should use it on our forum.
I've made a quick review. I'll continue it next week. For now:
- "Note: you may use CTRL + ALT + T hotkey to insert a tab character."
On my OS this opens a terminal :D And I started to think that we need to override TAB's navigation behaviour. To not conflict with dialog too much we can make it this way:
- When you focus
<textarea>
the screen reader says "In this field TAB inserts indentation characters. To unblock navigational role of TAB press ESC (or some shortcut)". From now on, until ESC is pressed, TAB inserts tabs (or other configurable characters). - When user presses ESC once, the tab is unblocked until he enters textarea again. Subsequent ESC press may also block TAB again, but not necessarily.
- We can change the tip below <textarea> so it says whether TAB is locked or not.
- Note that SHIFT+TAB should not be locked so if users gets stuck in the
<textarea>
(he could forget which key unblocks TAB), he can escape by backing away. - It's pretty easy to implement, because we may block TAB keystroke with high priority listener removing itself on ESC press.
- Why
height:440
in the samples? :D
- Sample is not added to samples index.
- The code snippet dialog is not very resizable. But it's not critical - it can be improved in the future. Now we should only fix dialog size on smaller resolutions, because it is affected by viewport height (only on page load though), but the minimal height is too big IMO.
- [Chrome at least] CTRL+R is locked when in the dialog.
CKEDITOR.plugins.codesnippet
lacks docstring.
- https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L219-L223 - better to use
parent.add( child )
.
- https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L201 - you cannot use
classes
- it's a garbage after ACF. It may not be available (e.g. if ACF was disabled).
- https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L145-L146 - is it ok? They look as not compatible and not precise.
- https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L185-L189 - do you have a TC for that?
- https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L191-L197 - it will be much better to use regexp to scan classes string ;).
- Should we handle somehow pasted
<pre><code>
elements? What'd happen if someone pasted a code snippet from another page?
- Code style:
- https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L105-L113 - indentation and missing new line before
defaults
. - https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L76 - too long.
comment:8 Changed 11 years ago by
Replying to Reinmar:
- "Note: you may use CTRL + ALT + T hotkey to insert a tab character."
On my OS this opens a terminal :D And I started to think that we need to override TAB's navigation behaviour. To not conflict with dialog too much we can make it this way:
- When you focus
<textarea>
the screen reader says "In this field TAB inserts indentation characters. To unblock navigational role of TAB press ESC (or some shortcut)". From now on, until ESC is pressed, TAB inserts tabs (or other configurable characters).- When user presses ESC once, the tab is unblocked until he enters textarea again. Subsequent ESC press may also block TAB again, but not necessarily.
- We can change the tip below <textarea> so it says whether TAB is locked or not.
- Note that SHIFT+TAB should not be locked so if users gets stuck in the
<textarea>
(he could forget which key unblocks TAB), he can escape by backing away.- It's pretty easy to implement, because we may block TAB keystroke with high priority listener removing itself on ESC press.
Ok, let's have TAB to insert tabs... but we should avoid breaking yet another key. So ESC is not a good option. Let's leave its default behavior to happen and instead have an additional keystroke to move out of the field. I would go with CTRL+. (period).
comment:9 Changed 11 years ago by
- Yea i agree that we might rethink tabulation / focus thing. In addition to that i think that it would be great idea to provide an access key for at least soude code textarea.
- It is ment to be only for testing phase - having it set to at least 440 i can see few widget instances without scrolling.
- -- will do --
- Then we'll need to put some extra attention over there. Currently i've simply copied
sourcedialog
code there. : ) - Hmmm chrome@Win7 does handle
ctrl + R
in default way - that's it will refresh the page. Please clarify behaviour that you'd expect. - -- will do --
regarding 12. That's actually a good question, i've checked it with chrome atm, and it does handle <pre><code>(...)</code></pre> elements out of a box, thought i did not test other browsers so far.
As for further code requests I will apply them, and ask for further feedback if will be needed.
Followup to point 1. We decided to:
- allow tab key to insert tabulation character
- bind next focus to
ctrl + .
(period) - leave
shift + tab
for browser default function (move focus to previous element)
comment:10 Changed 11 years ago by
Another part of feedback:
- better to use parent.add( child ) - done
- you cannot use classes - it's a garbage after ACF. It may not be available (e.g. if ACF was disabled). - done
- is it ok? They look as not compatible and not precise. - could you elaborate a little on that?
- do you have a TC for that? - pending
- it will be much better to use regexp to scan classes string ;). - done
- Code style: indentation and missing new line before defaults. - done
too long. - done
In addition to that i've noticed that undo of textarea gets messed up after tab insertion in a way that's currently done. We'll have to try do something about it.
comment:11 Changed 11 years ago by
Important info: for time being we dropped idea for enhanced tab support for textarea, coz initial implementation had bad integration with builtin undo (this within textarea).
There were more-or-less no problems with implementing that for Chrome/IEs, but FF was hard to fix, so we dropped it, to develop it later. All the code went to t/11480b branch.
comment:13 Changed 11 years ago by
Status: | review_failed → review |
---|
Plugin is ready for review, the only issue is FF problem, described in #11727, after it's being merged to major, we'll rebase onto it.
comment:16 Changed 11 years ago by
Ahh, and i think it would be great idea to limit toolbar in sample, coz user will have hard times with finding an icon. Adding following part to config:
toolbar: [ [ 'Source', 'Bold' ], [ 'CodeSnippet' ] ]
should do it.
comment:17 Changed 11 years ago by
Status: | review → review_failed |
---|
- No documentation and no tests for ajax.post.
- The configuration options should be called
codeSnippet_*
. - Code snippet plugin does not reuse the
editor.addContentsCss
method. - The code snippet styles which you add through addCss should land in a stylesheet file and be added using
editor.addContentsCss
. Additionally, they should not use.cke_*
class - they are style for people not our internal thing. So in other words - simply create generic pre element styles. ensurePluginNamespaceExists
does not have to exist - just create the namespace inbeforeInit
or even ininit
, because specific highlighters have to require codesnippet plugin anyway, so their code will be executed later.- https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L120 use function definition instead of function expression.
- Wrong type of comment: https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L186
- https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L190 use
setHtml
. You can also merge this case with the previous one and only process formattedCode conditionally. - The code execute this fuctnion for every single element in the editor: https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L214. It should only be called if we're in
<pre>
. The same applies to accessing theclass
property. - Why do we need
div.cke_snipper_wrapper
? https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L235-L237 - The default highlighter code should be pulled from codesnippet code - e.g. you can place it at the end. Now it's intermixed with the rest and it's hard to tell which part is codesnippet's part.
- There's an addClass method ;) https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L229
- Again, use function definition: https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L261
- To be removed: https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L318-L326
- Remember, remember about not putting empty line after
function() {
(the only exception may be some huge wrapping IIFE). - https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L42
- https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L36 You should use basic writer to http://docs.ckeditor.com/#!/api/CKEDITOR.htmlParser.element-method-writeChildrenHtml writeChildrenHtml].
- Use camel case: https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L51
- https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L79 Why is this an CKEDITOR's object? This should be placed in
editor.plugins.codesnippet
by simply defining these methods in plugin definition. The reason is that those methods modifies editor instance, not CKEDITOR object. - In the sample - make the "Remaining part is resposible.." (BTW. typo) more visible, because now it looks too much like editors' startup logic.
- The toolbars can indeed be simplified.
comment:18 Changed 11 years ago by
Owner: | changed from Marek Lewandowski to Olek Nowodziński |
---|---|
Status: | review_failed → assigned |
comment:20 Changed 11 years ago by
Development in branch:t/11480b (+tests). I refactorized all plugin files and tests.
What to do with codesnippetgeshi sample? (as the GeSHi library is out of repository).
Replying to Reinmar:
- No documentation and no tests for ajax.post.
Pending.
- The configuration options should be called
codeSnippet_*
.
Done.
- Code snippet plugin does not reuse the
editor.addContentsCss
method.
Done.
- The code snippet styles which you add through addCss should land in a stylesheet file and be added using
editor.addContentsCss
. Additionally, they should not use.cke_*
class - they are style for people not our internal thing. So in other words - simply create generic pre element styles.
Done.
ensurePluginNamespaceExists
does not have to exist - just create the namespace inbeforeInit
or even ininit
, because specific highlighters have to require codesnippet plugin anyway, so their code will be executed later.
Done.
- https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L120 use function definition instead of function expression.
Done.
- Wrong type of comment: https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L186
Done.
- https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L190 use
setHtml
. You can also merge this case with the previous one and only process formattedCode conditionally.
Done.
- The code execute this fuctnion for every single element in the editor: https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L214. It should only be called if we're in
<pre>
. The same applies to accessing theclass
property.
Done.
- Why do we need
div.cke_snipper_wrapper
? https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L235-L237
Done. I changed widget structure to pre > code
(just like input HTML).
- The default highlighter code should be pulled from codesnippet code - e.g. you can place it at the end. Now it's intermixed with the rest and it's hard to tell which part is codesnippet's part.
Done??
- There's an addClass method ;) https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L229
Done.
- Again, use function definition: https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L261
Done.
Done.
- Remember, remember about not putting empty line after
function() {
(the only exception may be some huge wrapping IIFE).
Done.
Done?
- https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L36 You should use basic writer to http://docs.ckeditor.com/#!/api/CKEDITOR.htmlParser.element-method-writeChildrenHtml writeChildrenHtml].
Done.
Done.
- https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L79 Why is this an CKEDITOR's object? This should be placed in
editor.plugins.codesnippet
by simply defining these methods in plugin definition. The reason is that those methods modifies editor instance, not CKEDITOR object.
Done.
- In the sample - make the "Remaining part is resposible.." (BTW. typo) more visible, because now it looks too much like editors' startup logic.
Done.
- The toolbars can indeed be simplified.
Done.
comment:21 Changed 11 years ago by
Some extra points which i made yesterday with Fred:
- before adding CKEDITOR.ajax.post to public, we must change its internal behavior, it should not perform json serialization on its input data, it should be mediaembed job to do so
- we need to remove geshi sample from
codesnippet
sample, but we may place an information about it, and a link to codesnipetgeshi at our add-ons repo - in codesnipetgeshi we need to remove geshi library itself, and provide a config variable (in this plugin) allowing to override library location
- we should create a file readme.md inside codesnippetgeshi with guides for developer, telling how to setup things. We might include php script content, which was initially placed inside that lib directory.
comment:22 Changed 11 years ago by
contents.css
- We should not add styles to <pre>, because it toches all PREs, including those which are not for the codesnippet plugin.
plugins/ajax/plugin.js
- CKEDITOR.ajax.post, should not JSON.stringify data internally before posting. It should instead post data as is. If data must be json, then this should be done before calling post(). (mentioned in comment:21)
comment:24 Changed 11 years ago by
Status: | review → review_failed |
---|
- Open sample, doubleclick snippet, edit code, close, you cannot undo that.
- Open sample, drag snippet, undo, you cannot redo.
- I think that both editors in the sample should use the same theme.
- I don't like the default hjs's theme... What do you think about choosing different one by default in sample? One with a dark bg of course ;). E.g. monokai_sublime?
- It's not mentioned in codesnippetgeshi's README where to get the GeSHi from.
pre
andpre>code.hjs
are defined in contents.css. While the second style cannot be kept there... why not stylingpre
by default in editor? Maybe it shouldn't be so drastic (dark bg), but some basic styling would be good and codesnippet would benefit from it too.
comment:25 Changed 11 years ago by
- No tests for codesnippetgeshi at all. No sample... How can we be sure that it works.
- The fact that
setHighlighter()
changes config object is... rather ugly. The languages are kept ineditor._.codesnippet.highlighter
so if setHighlighter is used then they should be used if config.codeSnippet_languages is not set. If config is set, then config should be used even if custom highlighter was defined. - It's better to add button and widget in
init
rather than inafterInit
. - There's no explanation in the inline editor section in the sample that developer needs to add theme's CSS manually.
- In the plugins.highlighter it's not explained what developer can do with that highlighter instance then. There should be link to setHightligher (and vice versa) and a code sample showing how to use setHighligher (it may be confusing where it is). To avoid duplication you can have sample in one method's doc and link to it from the second one.
- You're passing also languages property to highlighter constructor but it's not mentioned in the
@param def
. - Typo: https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L186
- Use copy, Luke. https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L304
- Why do we do this? https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L326 Please add a comment that it's a editable-only class. The config option doc could als explain what's it for.
- Missing spaces. https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L360
- Well... this is even funny: https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L363-L366
- https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L380 why not mention in setHighlighter/highlighter contstructor doc. A developer implementing his highlighter may stumble upon it there, not here.
comment:26 Changed 11 years ago by
Status: | review_failed → assigned |
---|
Changes pushed to branch:t/11480b (+tests)
Replying to Reinmar:
- Open sample, doubleclick snippet, edit code, close, you cannot undo that.
Pending.
- Open sample, drag snippet, undo, you cannot redo.
Pending.
- I think that both editors in the sample should use the same theme.
Done.
- I don't like the default hjs's theme... What do you think about choosing different one by default in sample? One with a dark bg of course ;). E.g. monokai_sublime?
Done.
- It's not mentioned in codesnippetgeshi's README where to get the GeSHi from.
Yes, it is. In the very first paragraph.
pre
andpre>code.hjs
are defined in contents.css. While the second style cannot be kept there... why not stylingpre
by default in editor? Maybe it shouldn't be so drastic (dark bg), but some basic styling would be good and codesnippet would benefit from it too.
We decided that styles for pre make no longer sense because they may collide
with other plugins. As for pre > code.hjs
, those styles depend on highlight.js and, if added via CKEDITOR.addCss
they would be wrong as there could be
some custom highlighter in use. pre > code.hjs
would fit only in codesnippethighlightjs
plugin, which would hold all the logic related to highlight.js and which, unfortunately, does not exist yet.
So the conclusion is that Code Snippet will not introduce any styles.
Replying to Reinmar:
- No tests for codesnippetgeshi at all. No sample... How can we be sure that it works.
Pending.
- The fact that
setHighlighter()
changes config object is... rather ugly. The languages are kept ineditor._.codesnippet.highlighter
so if setHighlighter is used then they should be used if config.codeSnippet_languages is not set. If config is set, then config should be used even if custom highlighter was defined.
It already works like that ;)
- It's better to add button and widget in
init
rather than inafterInit
.
Done.
- There's no explanation in the inline editor section in the sample that developer needs to add theme's CSS manually.
Done.
- In the plugins.highlighter it's not explained what developer can do with that highlighter instance then. There should be link to setHightligher (and vice versa) and a code sample showing how to use setHighligher (it may be confusing where it is). To avoid duplication you can have sample in one method's doc and link to it from the second one.
Done.
- You're passing also languages property to highlighter constructor but it's not mentioned in the
@param def
.
It has already been mentioned.
Done.
Done.
- Why do we do this? https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L326 Please add a comment that it's a editable-only class. The config option doc could als explain what's it for.
Done. I renamed the property to config.codeSnippet_codeClass
in case we wanted to introduce some class for <pre>
. Also extended docs.
Done.
Pending.
- https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L380 why not mention in setHighlighter/highlighter contstructor doc. A developer implementing his highlighter may stumble upon it there, not here.
Done.
comment:27 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Merged to major git:ae32d4c on dev cf045a1 on tests.
There are couple of topics, which we need to check before release - e.g. #11781, #11782, name in the elements path. Additionally, I found that because of limitations created by undo manager, we are forced to do very uncool and potentially dangerous for performance lock&unlock for every code snippet.
Pushed to snippet.
Config variables in snippet plugin:
<pre/>
tag inside editable (while editing)Knwon issues: