Skip to content
This repository was archived by the owner on Feb 6, 2023. It is now read-only.

Conversation

@howardjing
Copy link

This PR addresses the same concerns as #488 and #571. I saw a suggestion to add an inputTags option to blockRenderMap, but it didn't look like anybody had gotten around to implementing it.

This commit also introduces a failing test

it('must NOT treat divs as Ps when we pave Ps')

I wasn't sure of the background behind this test so I left it failing for now.

Summary

Allow user to specify inputTags as an array of tag names that will
get mapped to draft js block types. If inputTags are not specified,
fall back to element.

Change default blockRenderMap to support p and div as unstyled blocks.

For example, a blockRenderMap of

{
  'header-one': {
    element: 'h1',
    inputTags: ['h2, 'h3'],
  },
  header-two': {
    element: 'h2',
    inputTags: ['h1'],
  },
}

will map

<h2>Hello world!</h2>

and

<h3>Hello world!</h3>

to a header-one block, and will map

<h1>Hello world!</h1>

to a header-two block.

Test Plan

  1. Copy paste text with <p> tags into draft js editor. Each paragraph should be split up into a separate block.

Allow user to specify `inputTags` as an array of tag names that will
get mapped to draft js block types. If `inputTags` are not specified,
fall back to `element`.

Change default blockRenderMap to support `p` and `div` as unstyled blocks.

For example, a blockRenderMap of

```
{
  'header-one': {
    element: 'h1',
    inputTags: ['h2, 'h3'],
  },
  header-two': {
    element: 'h2',
    inputTags: ['h1'],
  },
}
```

will map

```
<h2>Hello world!</h2>
```

and

```
<h3>Hello world!</h3>
```

to a `header-one` block, and will map

```
<h1>Hello world!</h1>
```

to a `header-two` block.
.valueSeq()
.flatMap((config) => getInputTags(config))
.toSet()
.filter((tag) => tag && tag !== unstyledElement)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure why we were specifically filtering out unstyled blocks. Adding this filter back fixes the failing test, but it breaks the intended functionality of allowing p tags to be mapped to an unstyled block.

@mitermayer
Copy link
Contributor

Thanks for looking into this, I am unable to contribute to Draft until after the October 15th

@howardjing
Copy link
Author

Does anyone have any context as to what it must NOT treat divs as Ps when we pave Ps is testing? That's the one failing test in this PR -- everything else seems to be working correctly.

@howardjing
Copy link
Author

howardjing commented Sep 13, 2016

I dug into this a bit, and it looks like the failing test is converting html that looks like the following:

<div>
  <p>hello</p>
  <p>world</p>
</div>

My changes will generate a blockmap that looks like

{
  "fc4u6": {
    "key": "fc4u6",
    "type": "unstyled",
    "text": "hello",
    "characterList": [
      // ....
    ],
    "depth": 0,
    "data": { }
  },
  "efkb": {
    "key": "efkb",
    "type": "unstyled",
    "text": "world",
    "characterList": [
      // ....
    ],
    "depth": 0,
    "data": { }
  }
}

that looks like

image

whereas the current behavior will generate a blockmap that looks like

{
  "5s7os": {
    "key": "5s7os",
    "type": "unstyled",
    "text": "helloworld",
    "characterList": [
      // ....
    ],
    "depth": 0,
    "data": { }
  }
}

and looks like

image

which seems like a reasonable change.

@adamlong5
Copy link

@mitermayer who needs to sign off to get this merged?

@houkanshan
Copy link

houkanshan commented Oct 25, 2016

I thinks this PR should resolve #395 #465 #523 #738 . Also it has related opening PR #571 .

Our project is using a patched DraftJS based on version 0.9.1 with this PR merged. And it has been online for 1 month without any related bug report.

So if anyone could check the failing test and accept this PR, that would helps a lot!

@adamlong5
Copy link

@hellendag can you help here?

@mitermayer
Copy link
Contributor

Hi @howardjing, thanks for working on this issue, closing this since it has already been fixed on #828

@mitermayer mitermayer closed this Dec 15, 2016
@howardjing
Copy link
Author

@mitermayer cool! Thanks for closing this out.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants