T/27 #28
T/27 #28
Conversation
|
Related PR that should be handled simultaneously. |
| @@ -26,6 +26,6 @@ export default class Enter extends Feature { | |||
| this.listenTo( editingView, 'enter', ( evt, data ) => { | |||
| editor.execute( 'enter' ); | |||
| data.preventDefault(); | |||
| } ); | |||
| }, 'low' ); | |||
There was a problem hiding this comment.
This should be the standard observer. Others can use high priority.
There was a problem hiding this comment.
In engine, many callbacks are added with low priority, hinting that these are default callbacks that may be overwritten. In a result, most callbacks added in (external) plugins/features can be added with default priority and still overwrite default ones.
So, I felt like this is a very basic functionallity, almost like those in engine, hence I decided to go with low priority. If you really think this is wrong, I will change it. But remember that we have only 5 levels of events, so users will only get high and highest.
There was a problem hiding this comment.
OK, I can live with it. low makes some sense, cause indeed other listeners will usually want to precede it. And if anyone will want to do something after, there's still lowest.
There was a problem hiding this comment.
And there is low added after this listener (I am not sure now how this chain of dependencies and requirements in plugins will work, but I hope that Enter feature will be loaded as one of first ones.
There was a problem hiding this comment.
And there is low added after this listener
What do you mean?
I am not sure now how this chain of dependencies and requirements in plugins will work
Works like a charm :P. No other feature requires the enter feature right now, so it can be loaded first or last.
There was a problem hiding this comment.
I mean that if you add a listener with low priority after (in code) discussed listener, the new listener will be fired as a second one. So lowest is not the only option to go.
There was a problem hiding this comment.
So yes, if your feature requires the Enter feature, then it'd work.
| enterBlock( doc.batch(), doc.selection, { defaultBlockName: 'paragraph' } ); | ||
| enterBlock( batch, doc.selection ); | ||
|
|
||
| this.fire( 'afterExecute', { batch } ); |
There was a problem hiding this comment.
This must be automated. We can do this later ofc. See also my other comment about accepting batch and selection in many commands.
There was a problem hiding this comment.
This will be automated but doing it now would will take us too much time and it's better to close this part of code and then rework all the commands.
| */ | ||
| export function enterBlock( batch, selection, options = {} ) { | ||
| const defaultBlockName = options.defaultBlockName; | ||
| function enterBlock( batch, selection ) { |
There was a problem hiding this comment.
Why isn't it exposed any more? The command cannot be always reused (undo step integration isn't then possible), so the function can be useful from time to time.
This made me think that perhaps we should allow passing those params (batch, selection) to all commands which use them. In https://github.com/ckeditor/ckeditor5-enter/pull/28/files#r79292725 I wrote that we need to automate those kind of commands anyway, so perhaps we can go one step further and add also this flexibility.
There was a problem hiding this comment.
Why you cannot integrate undo steps? If the command adds the used batch to the data object passed with afterExecute event callback you can add new deltas to it.
Of course this way you cannot do beforeExecute and this could be an argument for making it more flexible.
Anyway, I don't think this need to be exposed anymore as calling this method is everything what EnterCommand does. There are no options, etc. So if you want to call this method, you can call editor.execute( 'enter' ) too. If you want to modify this, use afterExecute callback. My rule of thumb was "if Headings does not need it, it does not need to be exposed".
There was a problem hiding this comment.
At least one thing to fix: https://github.com/ckeditor/ckeditor5-enter/pull/28/files#r79292600
Plus couple of questions to answer and create followups.
Fixes ckeditor/ckeditor5#4555