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

bug: unable to remove header formatting in a specific scenario #5637

Open
patrickcate opened this issue Jul 24, 2021 · 14 comments
Open

bug: unable to remove header formatting in a specific scenario #5637

patrickcate opened this issue Jul 24, 2021 · 14 comments
Labels
good first issue status: confirmed type: bug code to address defects in shipped code

Comments

@patrickcate
Copy link

Is your feature request related to a problem? Please describe.
In the markdown editor toolbar, there is no easy way to remove heading level stylings once applied. From what I can tell, the only way to remove a heading level styling from text is to switch to the raw markdown mode and remove the # formatting there.

Describe the solution you'd like
Add a toolbar option to remove a heading styling if applied to selected text. I can thing of 3 ways this could be implemented.

  1. When a heading level option in the dropdown is applied/active, clicking the dropdown item again toggles the formatting off.
  2. Add a new heading dropdown option "Remove"
  3. Add a new toolbar button that more generally removes any applied markdown formatting from a selection.
@patrickcate patrickcate added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jul 24, 2021
@erezrokah
Copy link
Contributor

Hi @patrickcate, does clicking the header button again while at the end of the test works?
See
header

If not can you share a quick video or a step by step reproduction of what you're trying to accomplish?

@patrickcate
Copy link
Author

@erezrokah, yes clicking on the header again while at the end of the text does work. Didn't know that was possible.

It's good to know there is a way to remove the styling without having the go into the raw markdown editor mode, but I think it would be more intuitive if this also functioned when the text was highlighted and/or the cursor was inside the text.

@erezrokah
Copy link
Contributor

It's good to know there is a way to remove the styling without having the go into the raw markdown editor mode, but I think it would be more intuitive if this also functioned when the text was highlighted and/or the cursor was inside the text.

It should. Are you experiencing a different behavior? If so can you post a reproduction?

@patrickcate
Copy link
Author

It should. Are you experiencing a different behavior? If so can you post a reproduction?

OK, just tested (I'm just using the Netlify CMS demo site). It does work when the cursor is inside the text or you've select some text inside, but doesn't work when you select the entire line by triple clicking the text.

@erezrokah
Copy link
Contributor

header

This works for me. Can you share a step by step reproduction?

@patrickcate
Copy link
Author

patrickcate commented Jul 25, 2021

Here's a video showing the issue. When I'm selecting the entire line I'm triple clicking the text.

netflify-cms-header-dropdown.mov

@erezrokah erezrokah changed the title Add option to markdown editor to remove heading level formatting. bug: unable to remove header formatting in a specific scenario Jul 26, 2021
@erezrokah erezrokah added type: bug code to address defects in shipped code good first issue status: confirmed and removed type: feature code contributing to the implementation of a feature and/or user facing functionality status: more info needed status: unconfirmed labels Jul 26, 2021
@erezrokah
Copy link
Contributor

Thanks @patrickcate, I updated the issue title to match the video

@bytrangle
Copy link
Collaborator

bytrangle commented Jul 28, 2021

Hello @erezrokah , I know the reason for the issue.

When clicking the Heading button, for example Heading 1, we call the toggleBlock method. This method checks if every block in the selection is of type Heading 1. If it is, set all of them to default type aka paragraph, otherwise they remain heading 1.

This makes sense on paper, but the problem lies with triple click action in the Slate editor. Instead of the selecting just one block, internally it selects all the subsequent block. This means editor.value.blocks will contain more than one block.

Meanwhile, the UI indicates that only one block is selected, which leads to the confusion. The issue is well-documented here.

What do you suggest?

  1. Write our own triple-click implementation. In toggleBlock method, checks if it is triple click. If yes, select only the first block in the selection value. I haven't checked the feasibility of this approach though.
  2. Wait for the issue on Slate editor to get resolved, or do it myself. Then integrate Slate into Netlify CMS from scratch 😓 because the version we're using is no longer updated.

@erezrokah
Copy link
Contributor

Thanks @bytrangle, that makes sense.
So this issue will happen not just with headers correct?

  1. Write our own triple-click implementation. In toggleBlock method, checks if it is triple click. If yes, select only the first block in the selection value. I haven't checked the feasibility of this approach though.

I think this would be preferable, depending on the size of the effort.

2. Wait for the issue on Slate editor to get resolved, or do it myself. Then integrate Slate into Netlify CMS from scratch 😓 because the version we're using is no longer updated.

I've added this to #5652

@bytrangle
Copy link
Collaborator

bytrangle commented Jul 28, 2021

Good question. Interestingly, the List and Quote blocks are not affected because they are handled by different methods that use a different logic. For example:

<h1>Heading 1</h1>
<p>paragraph</p>

If you triple click Heading 1, the selection will be <h1> to </p>. editor.value.blocks will contain two blocks, heading-one and paragraph.

Then click either List or Quote button. The toggleQuoteblock and toggleList only look for editor.value.startBlock, which will always be the first block of the selection and then do the transformation.

In contrast, when clicking a Heading button, we look for editor.value.blocks and check if every single block inside it is of the same heading type with a helper method everyBlock. Of course they aren't, so this method returns false.

If everyBlock returns true, we pass defaultType = 'paragraph' to editor.setBlocks. But it returns false, so we pass the value of heading editor.setBlocks. So editor.setBlocks('heading 1') to a heading-one block doesn't change the same.

toggleBlock(editor, type) {
  switch(type) {
    case 'heading one':
    // ...
      return editor.setBlocks(editor.everyBlock(type) ? defaultType : type)
    // ...
  }
}

You may wonder: How about the paragraph block after <h1>? Does it get turned into heading-one? As I've said, editor.value.blocks contains both blocks, but when calling editor.setBlocks, Slate still correctly identifies “Heading 1” as the only block that gets selected. Hence the paragraph block is untouched.

I don't know why it behaves that way, but this is probably a reason why this triple-click issue has flown under the radar for so long.

But thanks to your inquiry, I think I've discovered an economical way to resolve this issue.

@bytrangle
Copy link
Collaborator

bytrangle commented Jul 28, 2021

@erezrokah While investigating the triple-click issue, I encounter this:

list-block-from-selection

I thought the expectation is to have both blocks be list items?

@erezrokah
Copy link
Contributor

I thought the expectation is to have both blocks be list items?

You're correct. This seems like a separate bug.

@bytrangle
Copy link
Collaborator

Has there been an issue filed for that? I've searched, but it's not easy to find the right keywords for these issues so I may have missed it. If there's none, I'll create one tomorrow and work on that one first.

@erezrokah
Copy link
Contributor

Has there been an issue filed for that?

There isn't! I should have mentioned it.

If there's none, I'll create one tomorrow and work on that one first.

That would be great, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue status: confirmed type: bug code to address defects in shipped code
Projects
None yet
Development

No branches or pull requests

4 participants
@patrickcate @erezrokah @bytrangle and others