Skip to content
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.

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

Create autoformatting engine and command with default config #2367

Closed
maxbarnas opened this issue Oct 4, 2016 · 14 comments · Fixed by ckeditor/ckeditor5-autoformat#2
Closed
Assignees
Labels
package:autoformat type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@maxbarnas
Copy link
Contributor

Create lists/headings when line started with # or * or \d. In the future should also create blockquotes.

It should be considered that this feature should not directly require other (link, heading, blockquote) features to exist. So integration must be performed very loosely. Either through a config (e.g. regexp/callback => command/callback) or a set of micro features (which, at this point, can require the link, heading, blockquote ones) which enable the autoformatting for their specific case.

If the engine of the autoformatter will be implemented correctly, the second idea (with specified classes) should be as doable as the declarative config. So the first thing to work on will be a flexible engine.

@maxbarnas maxbarnas self-assigned this Oct 4, 2016
@maxbarnas
Copy link
Contributor Author

maxbarnas commented Oct 4, 2016

When I remove prefix /#{1,3}/, I get error because function transform fails to get end property in argument range:

https://github.com/ckeditor/ckeditor5-engine/blob/master/src/model/liveposition.js#L149

This needs a further check, even though I "fixed" it by checking the existence of the end property. Any knowledge about a similar issue would be valuable.

@maxbarnas
Copy link
Contributor Author

Above problem was reported (https://github.com/ckeditor/ckeditor5-engine/issues/624) and fixed.

@maxbarnas
Copy link
Contributor Author

Some problems were solved and a couple of updates were made to other features to make Autoformat feature work.
But now, when there finally is a non-breaking example, I can clearly see that there is one flaw in our assumed structure of Autoformat. When, for example, I create numbered list, I still can invoke pattern for heading or bulleted list.

I think, that instead of separate new AutoformatEngine(...) calls we should create single AutoformatEngine and add patterns to that instance.
When a pattern is matched, we should "ask" other patterns to check if they are already applied to processed element.

@Reinmar
Copy link
Member

Reinmar commented Oct 7, 2016

This is not going to work (at least, not always). Features are independent of each other. It's only a convenience why we define Autoformat feature. But the engine can be then used by other features to add their new formatting experience.

But, in fact, I should first ask what's the actual TC? Because I don't see conflicts between those features. They should have exclusive regexp patterns and that's it.

@maxbarnas
Copy link
Contributor Author

As a TC I wrote:

When, for example, I create numbered list, I still can invoke pattern for heading or bulleted list.

Or in details (apostrophes added to stop MD from trimming spaces):

  1. Type '- ' and Autoformat changes current paragraph to bulleted list,
  2. When you try to type '- ' again, nothing will happen, because I check if we are already in the bulleted list,
  3. Type '1. ' in that bulleted item, Autoformat fires and changes bulleted item to numbered item.

My first idea how to solve it is to make Autoformat aware of patterns and provide with each pattern a check function to check if that particular pattern was applied in current paragraph.

So each feature provides a pattern, callback, and check function. This way every feature is still separated, but Autoformat has all the "tools" to decide when to apply formatting and when not.

@Reinmar
Copy link
Member

Reinmar commented Oct 10, 2016

What's wrong with behaviour "3"? How often you'd type "1. " at the beginning of a bulleted list? ;)

@maxbarnas
Copy link
Contributor Author

The same thing will happen with # and all future Autoformat patterns. If you think this is a minor issue, I will happily continue my work without worrying about it. :)

@Reinmar
Copy link
Member

Reinmar commented Oct 10, 2016

I think this is ok for now. We need MVP and we'll be able to see it in action. However, this is super clear that the support for undo must be correct, so such unwanted autoformatting can be undone.

@maxbarnas
Copy link
Contributor Author

Do you see any obvious situation where typing # at the beginning of heading is common? Because right now it will work like a toggle mechanism. I guess this should be fixed.

@wwalc
Copy link
Member

wwalc commented Oct 10, 2016

What's wrong with behaviour "3"? How often you'd type "1. " at the beginning of a bulleted list? ;)

  • 1.09.1939 - wybuch II wojny światowej

:P

@Reinmar
Copy link
Member

Reinmar commented Oct 11, 2016

There's no space after "1." in "1.09" ;)

Besides, you can always undo that step!

@Reinmar
Copy link
Member

Reinmar commented Oct 11, 2016

Besides2, we can change this behaviour easily to only work in paragraphs. Let's have a working solution first, though.

@maxbarnas
Copy link
Contributor Author

List of recent changes:

  • Simplified Autoformat feature,
    • Moved branch.remove() to AutoformatEngine,
    • Removed callbacks in favor of simple command names,
  • Removed loop checking every changed element in change event,
  • Removed needlessly complicated range creation,
  • Removed useless callback parameters,
  • Restricted Autoformat to work only in paragraphs,
  • Removed dead branches of code, that showed up after adding check above,
  • Updated tests.

@maxbarnas
Copy link
Contributor Author

TODO: After closing the ticket package.json in ckeditor5 should be updated with this package.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-autoformat Oct 9, 2019
@mlewand mlewand added this to the iteration 4 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:autoformat labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:autoformat type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants