fix(ui-markdown-editor): linebreak - #263, #267, #269 #270

d-e-v-esh · 2021-02-26T17:23:43Z

Closes #263 #267

Update

This was my thought process of finding the problem and the solution.

Identifying the problem

As you can see here, we do not have any function for the softbreak.
The functions that exist there are insertLinebreak and insertHeadingbreak.
So I thought that maybe softbreak was not working because there is no function for it. But that is actually not the case.
The softbreak was not working because of this conditional statement here. This basically means that if Enter is being pressed from a non-heading block then we totally skip out hotkey actions. That is also the reason why pressing Ctrl + Enter did not inset a page break. If you remove that statement then softbreak and page break works like normal.
And this should TOTALLY prevent us from getting a linebreak in a paragraph. And the only reason pressing Enter is still functional is because of this and this functions where we are overriding insertBreak(). You can put a few console.log() there and check which functions work and which do not.
That is also the reason why inserting a page break (Ctrl + Enter) and softbreak(Shift + Enter) works absolutely fine for all the heading blocks because this conditional statement only restricts non-heading blocks from all the Enter functions.

Exchange in types of `break` Bug

As I mentioned earlier in the second point that the only functions that existed are the insertLinebreak and insertHeadingbreak.
To find out which function is doing what, we need to remove this conditional statement here and simply add a console.log() statement after this line like this:

    hotkeys.forEach((hotkey) => {
      if (isHotkey(hotkey, event)) {
        event.preventDefault();
        const { code, type } = HOTKEYS[hotkey];
        console.log(code) <===
        hotkeyActions[type](code);
      }
    })

By doing this, we get:

Here:

Pressing Enter results in a => headingbreak
Pressing Shift + Enter results in a => linebreak

This is the case because this is how it is mentioned in the hotkeys file as you can see here.

And now we know that pressing Shift + Enter is creating a softbreak as it should but it is named as a linebreak and
Pressing Enter is always creating a false headingbreak no matter the type of block. It is false headingbreak and does not do anything because of the problem I mentioned above that is pressing Enter is useless in a non-heading block because of this conditional statement here

What is happening till now:
Every time we press Enter, a heading break is going to get inserted.

How it was implemented was:

We created a boolean which tells us if the current block is a heading.
We created a restriction on the enter keyword when being pressed from a non-heading block. This was causing Page break shortcut does not work consistently #263
If enter is called from a headingblock then we will go to our custom hotkeyActions and create a heading break
But if the enter is called from a non - heading block then we will skip our custom hotkeyActions and a default enter linebreak will occur

Changes

Converted headingbreak to linebreak
This converts every single enter press to a simple linebreak and if enter is pressed from a heading block, only then we will make it a headingbreak
This removes the restriction on the enter key in the paragraph so the page break works.

Related Issues

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

jolanglinais

We'll need to do a thorough investigation on all of this to see what is going on, the limitations, and what these changes do.

d-e-v-esh · 2021-03-01T17:47:44Z

@irmerk That is great. Will that take very long? Should I write the description of changes and limitations in more detail?

packages/ui-markdown-editor/src/components/index.js

packages/ui-markdown-editor/src/index.js

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

dselman · 2021-03-01T18:13:40Z

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

@@ -91,17 +90,26 @@ export const insertThematicBreak = (editor, type) => {
  Transforms.insertNodes(editor, tBreakNode);
 };

-export const insertLinebreak = (editor, type) => {
+export const insertSoftBreak = (editor, type) => {
  const text = { object: 'text', text: '' };
  const br = { type, children: [text], data: {} };


Why take type as an argument if this is just for soft break?

jolanglinais · 2021-03-04T16:43:16Z

@irmerk That is great. Will that take very long? Should I write the description of changes and limitations in more detail?

Yeah I think so. This is a sensitive change, so as much helpful context would be great.

d-e-v-esh · 2021-03-05T15:21:45Z

@dselman @irmerk I have updated the pull-request body and added more context highlighting the problem. Does this bring more clarity? Ask me any question that comes to your mind or regarding anything that was unclear.

dselman · 2021-03-08T11:27:26Z

When I test this using the Netlify preview there's something strange happening with the selection. Try selecting multiple paragraphs forwards, or backwards, and you see that the selection is getting extended outside of the selection target.

d-e-v-esh · 2021-03-09T02:15:21Z

@dselman I don't see that behavior. Can you make a screen recording to show exactly what is strange?

K-Kumar-01 · 2021-03-09T09:08:22Z

@d-e-v-esh
https://www.loom.com/share/56a2f8833f9f481e96dd5582fce0b745
I think @dselman may be talking about this thing.

dselman · 2021-03-09T10:07:17Z

@dselman I don't see that behavior. Can you make a screen recording to show exactly what is strange?

It seems to be specific to Safari - I don't see it on Chrome.

d-e-v-esh · 2021-03-09T16:01:19Z

@K-Kumar-01 Is this happening only on this update or is this on the master branch? You cut out the URL.

K-Kumar-01 · 2021-03-09T17:44:44Z

@K-Kumar-01 Is this happening only on this update or is this on the master branch? You cut out the URL.

@d-e-v-esh
It is happening in the deployment of this PR only.
I have rechecked it.
I clicked on the deploy preview and then made the above video.

d-e-v-esh · 2021-03-10T04:59:31Z

@K-Kumar-01 That is actually a really bad bug. I'll see what is causing the problem and update.

d-e-v-esh · 2021-03-10T06:57:09Z

@dselman Can you show which merge started converting blank lines to \ in markdown that you talked about here down in #269? I think that could be the one causing this problem. Because that was not happening in the initial commit and that did not happen when I run it locally until a did a git pull and now nothing I do reverses this thing.

jolanglinais

We still haven't found time yet, but this needs investigation by @accordproject/maintainers-ui.

In the meantime @d-e-v-esh you could rebase all your commits to clean them up and put DCO signoffs on them.

d-e-v-esh · 2021-03-17T11:13:22Z

@irmerk Does this look good now?

jolanglinais · 2021-03-17T11:50:10Z

Yes looks good. I think wait to rebase again (because there are conflicting files after I merged a couple other PRs) until we review.

jolanglinais · 2021-04-01T15:17:51Z

I've finally been able to look more into this. This is really thorough and I think is on the right track. Some residual awkward behavior I'm seeing is when you press ENTER at the end of "My Heading" at the top of the demo, it inserts two new paragraphs, both of which are not headings, and places the cursor at the beginning of the second one. Could this be because we are inserting lineBreakNode in insertLineBreak?

jolanglinais · 2021-04-06T13:52:50Z

OH yes! Good point. There's so many related Issues/PRs here I'm getting confused. Okay great, even better. This PR looks good so I'm going to merge!

jolanglinais · 2021-04-06T14:10:22Z

@d-e-v-esh we're going to put the changes in this PR on the working group call tomorrow to try to make sure it doesn’t lead to further confusion around linebreaks/softbreaks. If you’ll be on during the call, feel free to add to the conversation. If not, I think @dselman and I can present what the changes will do.

dselman

This looks very clean - however when I try the Netlify preview and I forward-select past a line break something strange happens with the selection (whole blocks get selected). I can't reproduce the same issue with the master: https://ap-web-components.netlify.app/?path=/story/markdown-editor--demo

d-e-v-esh · 2021-04-07T10:27:28Z

@dselman I think the reason for that seems to be this that I talked about earlier. It seems like something changed in the conversion. That behavior is not present in the master and it was also not present when I made the PR. Do you know where it could be coming from?

dselman · 2021-04-07T14:52:43Z

@dselman I think the reason for that seems to be this that I talked about earlier. It seems like something changed in the conversion. That behavior is not present in the master and it was also not present when I made the PR. Do you know where it could be coming from?

Sorry, no idea. Can you reproduce the issue when you run locally using this branch?

d-e-v-esh · 2021-04-07T17:43:07Z

@dselman Yes I can. It doesn't go back to when this was not happening.

jolanglinais · 2021-04-09T14:46:40Z

Update: This PR should no longer address #267

jolanglinais · 2021-04-09T14:51:14Z

Also, this should unblock #260 and #284

d-e-v-esh · 2021-04-14T12:23:54Z

@irmerk I have implemented the new paragraphBreak and also fixed the error that @dselman pointed out. I think it should be perfect now. Now we just need to update the demo text on the editor.

jolanglinais

Yes, this looks good to me! Will want @dselman on this as well.

jolanglinais · 2021-05-05T15:49:24Z

Pinging @dselman on this again.

dselman · 2021-05-18T13:39:34Z

@d-e-v-esh looks great. Can you please resolve the merge conflicts and then I will merge.

dselman

Looks great. Please resolve the merge conflicts and I will merge.

dselman · 2021-06-03T10:01:34Z

Would love to get this into the next release (this week). @d-e-v-esh will you have time to resolve the merge conflicts?

adithyaakrishna · 2021-10-20T10:47:56Z

@d-e-v-esh Any Updates on this? If not, I would love to fix it up and send a PR

d-e-v-esh · 2021-10-20T11:26:07Z

@adithyaakrishna Got extremely busy. I'll get to it in a few days. You can find other issues. This PR is complete, just needs the merge conflicts to be resolved.

…rdproject#260 Signed-off-by: d-e-v-esh <[email protected]>

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

d-e-v-esh · 2021-10-31T15:24:04Z

@dselman I removed all the merge conflicts and it works totally great. You can check it out.

Michael-Grover

lgtm, thanks @d-e-v-esh !

Michael-Grover requested review from jeromesimeon, dselman, jolanglinais and Michael-Grover February 26, 2021 18:02

Michael-Grover assigned d-e-v-esh Feb 26, 2021

jolanglinais added the Type: Need Investigation 🔬 label Mar 1, 2021

jolanglinais suggested changes Mar 1, 2021

View reviewed changes

dselman requested changes Mar 1, 2021

View reviewed changes

d-e-v-esh mentioned this pull request Mar 5, 2021

Enter on Heading Text - Cursor Jump #260

Open

d-e-v-esh mentioned this pull request Mar 9, 2021

Linebreak in lists do not work correctly #284

Open

d-e-v-esh mentioned this pull request Mar 10, 2021

feat(ui-markdown-editor): direct page break - #276 #288

Merged

9 tasks

jolanglinais mentioned this pull request Mar 12, 2021

Soft Break not Working => Line Break and Soft Break are interchanged in the code #267

Closed

jolanglinais suggested changes Mar 16, 2021

View reviewed changes

d-e-v-esh force-pushed the master branch 3 times, most recently from 77aa58b to 495045d Compare March 17, 2021 11:11

jolanglinais approved these changes Apr 6, 2021

View reviewed changes

dselman requested changes Apr 6, 2021

View reviewed changes

d-e-v-esh force-pushed the master branch from 702d2e4 to b75e8d4 Compare April 13, 2021 13:36

jolanglinais requested a review from dselman April 14, 2021 13:13

jolanglinais approved these changes Apr 14, 2021

View reviewed changes

d-e-v-esh mentioned this pull request May 6, 2021

Change in Demo Text Of Markdown Editor #339

Open

dselman requested changes May 18, 2021

View reviewed changes

fix(ui-markdown-editor): fixes cursor movement around headings - acco…

0def6ec

…rdproject#260 Signed-off-by: d-e-v-esh <[email protected]>

d-e-v-esh force-pushed the master branch 2 times, most recently from dc4c014 to e9e71ec Compare October 31, 2021 15:01

fix(ui-markdown-editor): linebreak - accordproject#263, accordproject…

4df5487

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

d-e-v-esh force-pushed the master branch from 2842b21 to 4df5487 Compare October 31, 2021 15:11

jolanglinais requested a review from dselman November 30, 2021 21:30

jolanglinais added Type: Enhancement ✨ Improvement to process or efficiency and removed Type: Need Investigation 🔬 labels Nov 30, 2021

Michael-Grover approved these changes Dec 1, 2021

View reviewed changes

fix(ui-markdown-editor): linebreak - #263, #267, #269 #270

Are you sure you want to change the base?

fix(ui-markdown-editor): linebreak - #263, #267, #269 #270

Conversation

d-e-v-esh commented Feb 26, 2021 • edited Loading

Closes #263 #267

Update

Identifying the problem

Exchange in types of break Bug

Here:

Changes

Related Issues

Author Checklist

jolanglinais left a comment

Choose a reason for hiding this comment

d-e-v-esh commented Mar 1, 2021 • edited Loading

dselman Mar 1, 2021

Choose a reason for hiding this comment

jolanglinais commented Mar 4, 2021

d-e-v-esh commented Mar 5, 2021 • edited Loading

dselman commented Mar 8, 2021

d-e-v-esh commented Mar 9, 2021

K-Kumar-01 commented Mar 9, 2021

dselman commented Mar 9, 2021

d-e-v-esh commented Mar 9, 2021

K-Kumar-01 commented Mar 9, 2021

d-e-v-esh commented Mar 10, 2021

d-e-v-esh commented Mar 10, 2021 • edited Loading

jolanglinais left a comment

Choose a reason for hiding this comment

d-e-v-esh commented Mar 17, 2021

jolanglinais commented Mar 17, 2021

jolanglinais commented Apr 1, 2021

jolanglinais commented Apr 6, 2021

jolanglinais commented Apr 6, 2021

dselman left a comment

Choose a reason for hiding this comment

d-e-v-esh commented Apr 7, 2021

dselman commented Apr 7, 2021

d-e-v-esh commented Apr 7, 2021

jolanglinais commented Apr 9, 2021

jolanglinais commented Apr 9, 2021

d-e-v-esh commented Apr 14, 2021

jolanglinais left a comment

Choose a reason for hiding this comment

jolanglinais commented May 5, 2021

dselman commented May 18, 2021

dselman left a comment

Choose a reason for hiding this comment

dselman commented Jun 3, 2021

adithyaakrishna commented Oct 20, 2021

d-e-v-esh commented Oct 20, 2021 • edited Loading

d-e-v-esh commented Oct 31, 2021

Michael-Grover left a comment • edited Loading

Choose a reason for hiding this comment

d-e-v-esh commented Feb 26, 2021 •

edited

Loading

Exchange in types of `break` Bug

d-e-v-esh commented Mar 1, 2021 •

edited

Loading

d-e-v-esh commented Mar 5, 2021 •

edited

Loading

d-e-v-esh commented Mar 10, 2021 •

edited

Loading

d-e-v-esh commented Oct 20, 2021 •

edited

Loading

Michael-Grover left a comment •

edited

Loading