accordproject / web-components

React Components for Accord Project
Apache License 2.0
117 stars 94 forks source link

feat(ui-markdown-editor): inline wysiwyg - #360 #361

Open d-e-v-esh opened 2 years ago

d-e-v-esh commented 2 years ago

Closes #360

This PR will let people directly write formatted text with the help of markdown shortcuts.

It adds the following features:

  1. Code Inline → `code inline` → code inline
  2. Italic Text → *italic text* or _italic text_ → italic text
  3. Bold Text → **bold text** or => __bold text__ → bold text
  4. Bold and Italic Text → ***bold and italic text*** or ___bold and italic text___ → bold and italic text

Changes

Functions added:

Bugs and Improvements

Screenshots or Video

https://user-images.githubusercontent.com/59534570/146959271-4c2064f7-8aa1-4226-a3b2-132144a9e977.mp4

Related Issues

Author Checklist

irmerk commented 2 years ago

Just an initial thought from watching the recording: I think it would be better to have the text formatted after the final character of the formatting text rather than the space after.

So instead "bold" turning into "bold" when the cursor (_) is in this location:

typing *bold* _

Have "bold" turn into "bold" when the cursor (_) is in this location:

typing *bold*_
d-e-v-esh commented 2 years ago

Really great stuff here!

Some initial thoughts. After going through these, could you also format the code in the files (especially packages/ui-markdown-editor/src/utilities/matchCases.js) so it's a bit easier to go through?

I likely have more thoughts, but I think with the formatting and my already made comments, it's getting a bit hectic so I'll add more after you address these comments.

@irmerk The formatting was looking normal in VSCode but for some reason, it started to look weird after I pushed it here, so I ran it through prettier and now it looks normal.

d-e-v-esh commented 2 years ago

Just an initial thought from watching the recording: I think it would be better to have the text formatted after the final character of the formatting text rather than the space after.

  1. So instead "bold" turning into "bold" when the cursor (_) is in this location:

typing *bold* _
  1. Have "bold" turn into "bold" when the cursor (_) is in this location:

typing *bold*_

@irmerk I think the two ways of executing shortcuts that you mentioned above have very different implementations. I implemented the (1) one which seemed a bit easier for me.

How it works here is:

  1. When the function runs on the press of SPACE_CHARATER, the matchBoldAndItalic simply counts the asterisks (*) or underscores ( _ ) at the end of that matched regex, i.e.

    If I write this:

    This is **bold text**

    The matched regex will be:

    **bold text**

    And the function will count 2 as the number of asterisks at the end **bold text|**| and turn it in bold text.

  2. If I just remove that onkeyup condition, and run the matchCases function on every keypress then it will not work.

    If I write this: =>

    This is **bold text**

    Before it even becomes into bold text, it will match the italicMatchCase and turn the text into italic text and the result will be something like:

    This is *bold text.

This will happen before we even enter the second asterisk at the end. It will just take *bold text from the **bold text* and convert it to italic.


For it to behave how you mentioned (2), I think I'll probably have to scrap this method and implement it the other way. I think it'll work like how stack data structure works in a compiler, where it'll keep track of the number of * and _ that has been entered onto the current line.

In this implementation, we will count the * and _ at the beginning |**|bold text** of the word instead of the end **bold text|**| and appropriately run those matching functions.

Let me know what you think about it or if you think there is some other way. 👍

irmerk commented 2 years ago

Apologies for the delay in responding.

This is interesting, I hadn't thought of that. **bold** will turn italic because of the first * after bold will be registered and we'll never be able to get to the second * to actually make it bold. Hmm... It would work if we used _ for italic:

_word_ = word *bold* = bold

But we're working with CommonMark.

I think I'd like a UX perspective here before proceeding in any direction, if you can look at this @Michael-Grover.

d-e-v-esh commented 2 years ago

@irmerk Yes, that'll be great. I'll wait for that before I update anything 👍

d-e-v-esh commented 2 years ago

@dselman

Could this be moved to its own function - like withShortcuts does in the Slate example? https://github.com/ianstormtaylor/slate/blob/main/site/examples/markdown-shortcuts.tsx

Yes, I think that it is a good idea. We can do that.

I also notice that the Slate example covers more shortcuts than this does.

This PR just includes the shortcuts that are performed on inline text, will add more shortcuts once this gets approved.

They also hook into insertText which I suspect means that the code will also work on copy/paste. Thoughts?

These inline shortcuts also hook into the insertText so they will totally work on copy/paste.

Would love to know what others think?

irmerk commented 2 years ago

I agree that we can iterate on more shortcuts after this gets merged in. I think a withShortcuts approach would be great.

d-e-v-esh commented 2 years ago

I agree that we can iterate on more shortcuts after this gets merged in. I think a withShortcuts approach would be great.

Thanks for the feedback. I'll update the PR with a withShortcuts approach soon. 👍

d-e-v-esh commented 2 years ago

@irmerk @dselman I implemented it with the withShortcuts approach. Does it need any more changes?