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

d-e-v-esh · 2021-12-21T16:50:21Z

Closes #360

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

It adds the following features:

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

Changes

Functions added:

matchCodeInline => Implements the markdown shortcuts for inline code
matchBoldAndItalic => Implements the markdown shortcuts for italic, bold and bold + italic text.
insertFormattedInlineText => This function is used in both the above functions. It is used to format and insert the formatted text in the right place.

Bugs and Improvements

A bug was also fixed from the previous PR where all the matching functions would run on every keypress as soon as the Regex matched, now the matching functions run only after the spacebar is pressed. This makes using the shortcuts much more easier and convenient.
Removed editor and currentLine arguments from functions as those values are available in the whole file and can be fetched directly by those functions.

Screenshots or Video

2021-12-21.20-40-30.mp4

Related Issues

Issue Magic Shortcuts for Markdown #321
Pull Request Heading Markdown Shortcut WYSIWYG (What You See Is What You Get) #346

Author Checklist

Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
Vital features and changes captured in unit and/or integration tests
Commits messages follow AP format
Extend the documentation, if necessary
Merging to master from fork:branchname
Manual accessibility test performed
- Keyboard-only access, including forms
- Contrast at least WCAG Level A
- Appropriate labels, alt text, and instructions

Signed-off-by: Devesh <[email protected]>

jolanglinais · 2021-12-21T18:02:50Z

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*_

jolanglinais

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.

jolanglinais · 2021-12-21T18:06:22Z

packages/ui-markdown-editor/src/utilities/constants.js

@@ -4,6 +4,8 @@ const MISC_CONSTANTS = {
  DROPDOWN_NORMAL: 'Normal',
 };

+export const SPACE_BAR = " ";


Slightly nit-picky, but maybe this variable could be SPACE_CHARACTER instead.

I changed it to a SPACE_CHARACTER. 👍

jolanglinais · 2021-12-21T18:10:07Z

packages/ui-markdown-editor/src/plugins/withText.js

+			switch (ev.key) {
+  	    case SPACE_BAR:
+					matchCases(editor, currentLine);
+				break;
+			}
+		}	


Do you expect there to be other cases? If not, I think we could do this without a switch case to be more simple.

I changed it to a simple if-else statement as I don't think there will be more cases for it right now.

jolanglinais · 2021-12-21T18:13:25Z

packages/ui-markdown-editor/src/plugins/withText.js

+
+		const currentLine = currentNode.text
+
+		onkeyup = (ev) => {


Could this be better handled in our onKeyDown handler instead of in the withText plugin?

jolanglinais · 2021-12-21T18:15:53Z

packages/ui-markdown-editor/src/utilities/matchCases.js

+	/**
+	 * 
+	 * @param {string} textToInsert The text that we want to format
+	 * @param {Object} textFormats This is the format style of the text (bold, italic, code) we want to apply or remove as the key in an object
+	 * and a boolean as the value. i.e. `{ bold: true }`
+	 * @param {Integer} markdownCharacters The number of markdown characters that the user has to type to trigger WYSIWYG
+	 */
+	const insertFormattedInlineText = (textToInsert, textFormats, markdownCharacters) => {


Could insertFormattedInlineText be defined outside of matchCases?

Should I put it outside the matchCases function or outside the matchCases file?

I think outside the function, probably keep it in this file for now.

Signed-off-by: Devesh <[email protected]>

d-e-v-esh · 2021-12-22T09:20:00Z

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 · 2021-12-22T09:59:37Z

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*_

@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:

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.
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. 👍

jolanglinais · 2022-01-14T18:18:14Z

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 · 2022-01-16T15:03:14Z

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

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

I also notice that the Slate example covers more shortcuts than this does. They also hook into insertText which I suspect means that the code will also work on copy/paste. Thoughts?

d-e-v-esh · 2022-01-20T13:53:31Z

@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?

jolanglinais · 2022-01-21T20:15:51Z

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 · 2022-01-22T11:45:20Z

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. 👍

Signed-off-by: Devesh <[email protected]>

d-e-v-esh · 2022-01-25T12:45:12Z

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

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

945da9c

Signed-off-by: Devesh <[email protected]>

jolanglinais assigned d-e-v-esh Dec 21, 2021

jolanglinais added Difficulty: Challenging Package: Markdown Editor ⌨️ Type: Feature Request 🛍️ New feature or request labels Dec 21, 2021

jolanglinais reviewed Dec 21, 2021

View reviewed changes

feat(ui-markdown-editor): updates - accordproject#360

1bc8bd8

Signed-off-by: Devesh <[email protected]>

dselman reviewed Jan 19, 2022

View reviewed changes

Implemented withShortcuts

90f7ef8

Signed-off-by: Devesh <[email protected]>

jolanglinais requested a review from dselman January 28, 2022 17:13

jolanglinais requested review from DianaLease and mttrbrts February 4, 2022 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

d-e-v-esh commented Dec 21, 2021

jolanglinais commented Dec 21, 2021

jolanglinais left a comment

jolanglinais Dec 21, 2021

d-e-v-esh Dec 22, 2021

jolanglinais Dec 21, 2021

d-e-v-esh Dec 22, 2021

jolanglinais Dec 21, 2021

jolanglinais Dec 21, 2021

d-e-v-esh Dec 22, 2021 •

edited

Loading

jolanglinais Jan 12, 2022

d-e-v-esh commented Dec 22, 2021 •

edited

Loading

d-e-v-esh commented Dec 22, 2021 •

edited

Loading

jolanglinais commented Jan 14, 2022

d-e-v-esh commented Jan 16, 2022 •

edited

Loading

dselman left a comment

d-e-v-esh commented Jan 20, 2022 •

edited

Loading

jolanglinais commented Jan 21, 2022

d-e-v-esh commented Jan 22, 2022

d-e-v-esh commented Jan 25, 2022

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

Are you sure you want to change the base?

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

Conversation

d-e-v-esh commented Dec 21, 2021

Closes #360

Changes

Functions added:

Bugs and Improvements

Screenshots or Video

Related Issues

Author Checklist

jolanglinais commented Dec 21, 2021

jolanglinais left a comment

Choose a reason for hiding this comment

jolanglinais Dec 21, 2021

Choose a reason for hiding this comment

d-e-v-esh Dec 22, 2021

Choose a reason for hiding this comment

jolanglinais Dec 21, 2021

Choose a reason for hiding this comment

d-e-v-esh Dec 22, 2021

Choose a reason for hiding this comment

jolanglinais Dec 21, 2021

Choose a reason for hiding this comment

jolanglinais Dec 21, 2021

Choose a reason for hiding this comment

d-e-v-esh Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

jolanglinais Jan 12, 2022

Choose a reason for hiding this comment

d-e-v-esh commented Dec 22, 2021 • edited Loading

d-e-v-esh commented Dec 22, 2021 • edited Loading

jolanglinais commented Jan 14, 2022

d-e-v-esh commented Jan 16, 2022 • edited Loading

dselman left a comment

Choose a reason for hiding this comment

d-e-v-esh commented Jan 20, 2022 • edited Loading

jolanglinais commented Jan 21, 2022

d-e-v-esh commented Jan 22, 2022

d-e-v-esh commented Jan 25, 2022

d-e-v-esh Dec 22, 2021 •

edited

Loading

d-e-v-esh commented Dec 22, 2021 •

edited

Loading

d-e-v-esh commented Dec 22, 2021 •

edited

Loading

d-e-v-esh commented Jan 16, 2022 •

edited

Loading

d-e-v-esh commented Jan 20, 2022 •

edited

Loading