bustle / mobiledoc-kit

A toolkit for building WYSIWYG editors with Mobiledoc
https://bustle.github.io/mobiledoc-kit/demo/
MIT License
1.55k stars 150 forks source link

fixes #642 #643

Closed miguelcobain closed 6 years ago

miguelcobain commented 6 years ago

Failing test for #642.

Adding this._textInputHandler.handle(key.toString()); on the event manager like we did for tabs doesn't work. Brings up a lot of new errors.

miguelcobain commented 6 years ago

Added a commit that fixes the previously added failing test and doesn't break the rest of the test suite.

It's a bit "hacky", but the problem at hand is also too specific.

Let me know if you want me to change something.

mixonic commented 6 years ago

@miguelcobain It seems inconsistent that other triggers for matching, like \t, are not included in the matching text but \n is.

I think triggering the handler search on \n makes sense, 100%.

But perhaps we should not pass the \n through at all. The matcher in #642 would then be /abc/ or /abc$/, but would trigger on \t as well as \n.

The alternative would be to consistently pass the trigger (\t, \n etc) to the handler.

miguelcobain commented 6 years ago

An example for using matcher /abc\n/ is to "linkify" links (see this blog post https://medium.com/@bantic/use-mobiledoc-kit-to-build-an-auto-linking-text-editor-771bdb0b8709)

The matcher there is /\b(https?:\/\/[^\s]+)\s$/. It ends with \s which is a superset of \n. The matcher in the blog post never executes when you type a link and then press enter (with no space before).

That case is what my test tries to cover.

mixonic commented 6 years ago

Ah @miguelcobain I think I'm just wrong. \t is passed as part of the string as you can see here: https://github.com/bustle/mobiledoc-kit/pull/643/files#diff-72682ed79dd0c75942760b33f6312299R293

Yeah there is something funky about \n being in the matched string when we never actually have a \n in the document though. For example a user might expect /a\nb/ to match across sections when it would not. \n is really acting as a special character for just for matches.

@miguelcobain can you update the documentation to mention the unique nature of \n and I can get this merged/released? Looks good.

miguelcobain commented 6 years ago

For example a user might expect /a\nb/ to match across sections when it would not. \n is really acting as a special character for just for matches.

Exactly. 😢 That's what I meant by "problem at hand is also too specific".

To cover cases such as /a\nb/ we would perhaps have to keep a copy of the document where \n is preserved or something. Don't know enough about mobile doc to fully understand the implications of that.

Where would you expect the documentation to be updated? Perhaps something here: https://github.com/bustle/mobiledoc-kit#responding-to-text-input

saying that things like /a\nb/ are not possible?

mixonic commented 6 years ago

@miguelcobain I went ahead and threw something together: https://github.com/bustle/mobiledoc-kit#responding-to-text-input

Thanks for this!