code-block in nested inside of two pre now by default

[ianstormtaylor]

I think this might be related to the @mitermayer? It looks like all code blocks are nested inside of two wrapping <pre> elements, instead of just one like before. (Tested on the Draft.js example on the website.)

[mitermayer]

@ianstormtaylor, that looks like a bug, i think in order to fix it we need to:

A a solution for it would be to make the 'element' property from the blockRenderMap only responsible for dealing with reverse enginering pasted and converted from html blocks into their respective blockTypes and add a another option that would then define the render. This option could then be nullable which would be similar to the problem on issue: #372

[mitermayer]

This is happening because we have code-block using a wrapper and element at the same time.

src/model/immutable/DefaultDraftBlockRenderMap.js

 'code-block': {
 68     element: 'pre',
 69     wrapper: PRE_WRAP,
 70     nestingEnabled: false
 71   },

We could just add another option maybe something like renderElement this could then be a string for wraping with html, or a react component. This way could even get rid of getBlockStyle() and making it also part of the blockRenderMap.

what do you guys think about this ?

[ianstormtaylor]

For what it's worth, I think element would be more clear as "match" or similar that conveys it's actually just used to find an element from pasted html.

But even then, I think it would also be better as a function that could perform slightly more complex logic, for example figure would be used for images, videos, or any other type of block, and just matching by element type isn't really enough.

The current system for dealing with block rendering feels really convoluted. I personally like the idea of streamlining more, like you're talking about.

Is there a reason why blockRenderMap has to be an immutable map? Instead of just a regular dictionary? It makes it a lot more kludgy to work with.

[hellendag]

@ianstormtaylor: As I recall, blockRenderMap is immutable for quick object comparison and reliable rendered output. It looks like we may have forgotten to add a corresponding check to shouldComponentUpdate, but something should probably be there. :) What kind of kludginess are you encountering? Mostly just dealing with the immutable API?

We were using functions before, to define the wrapper and rendered elements (i.e. https://github.com/facebook/draft-js/blob/5c957ad75d299fd77b098bed3965f753defc4d9a/src/component/utils/getElementForBlockType.js). This was separate from how block types were extracted from pasted content. To clarify, is the suggestion to move back in this direction but make this behavior customizable? (It was previously baked into DraftEditorContents.)

[ianstormtaylor]

I don't have a concrete suggestion either way, since I'm not familiar enough with the internal code, but general feeling is that dealing with "custom blocks" is a convoluted right now, since it seems like there are so many overlapping places that pieces need to be defined for them:

1. blockRendererFn to define how the blocks render.
2. blockRenderMap to define which block to render from copy-paste. (And other scenarios?)
3. blockStyleFn to add a class name to a block when rendered.
4. A way to parse out block data from your serialized data format.
5. A way to serialize block data into your format of preference.

Note: requirements 4 and 5 are technically "outside of Draft's realm", but I feel like they should be treated much more like core responsibilities (not necessarily supported by the codebase, but supported by the architecture) since it seems prudent to not couple your data storage with Draft's opinionated way of structuring the data for interoperability and other reasons.

It feels like each of these things is being considered in isolation. And the system could really be boiled down to a simpler model where you have a few things:

1. A type name for the block in question.
2. A component to render. (This would subsume blockStyleFn, blockRendererFn, and part of blockRenderMap.)
3. A way to deserialize the type from other formats (eg. paste). (This would subsume the other part of blockRenderMap. Also, this should be more flexible than purely recognizing by tagName, because that's not going to be enough to distinguish different types of blocks.)

I think those are the only two requirements, maybe there are more. (It seems like?) they could be defined in a single place, as a single piece of data—canonically—so that it doesn't feel like we have to bandaid together things from all different parts of the API.

I might be misunderstanding how they are supposed to work together though. I had to migrate back to 0.6.0 because I couldn't get things working the way I wanted after trying for a while.

[ianstormtaylor]

I'd personally love to go more towards the direction of having all of the built-in block types be composed from the same types of custom block definitions, since I think doing that would make that code/architecture path handled more as a first-class use case.

[joshmarinacci]

Since we are talking about restructuring blocks, I'd like the ability to create a custom type. Meaning it has a new string when you call getBlockType() and I can control the generated DOM and styling. I think this is possible today by using an atomic block type with a custom renderer, but that doesn't feel right if the new block type is still editable like any traditional block.

[mitermayer]

@ianstormtaylor I raised this issue #486 in order to track down a potential solution and improvement of the current behaviour. Would be great to have your suggestions on how to deal / tackle that problem in there as well