adding-support-for-paragraph - making blockTypes configurable #205

mitermayer · 2016-03-14T05:43:54Z

This fixes issue #204

Including paragraph on converter
Updating website and demo
Updating example

…t module - Including paragraph on converter - Updating website and demo - Updating example

mitermayer · 2016-03-14T05:44:33Z

examples/rich/rich.html

        {label: 'H1', style: 'header-one'},
        {label: 'H2', style: 'header-two'},
+        {label: 'H3', style: 'header-three'},
+        {label: 'H4', style: 'header-four'},


added the extra missing headers from the rich example

hellendag · 2016-03-14T15:39:24Z

cc @CLowbrow -- will it be a problem to have p map to paragraph in the paste processor?

mitermayer · 2016-03-14T22:58:47Z

will let you guys decide, in the meantime i noticed some tests are broken fixing them now and amending the PR. Will move the missing headers from this PR to the other PR since it makes more sense in #207 then here

…since it has been added on another pull request - Removed the inclusion of the extra headers from the rich.html since a new PR is dealing with it #207

CLowbrow · 2016-03-14T23:05:39Z

@hellendag stuff broke with some renderers when we added the H4/5/6 stuff right? I'm worried the same stuff is going to happen here. Do our renderers know how to deal with this type? I'm half-remembering all of this, though.

Right now 'unstyled' basically means paragraph. We can change it, but then what would unstyled be? Is it redundant?

- When we used paragraph as an invalid element to generate unstyled swapped that to use div instead

mitermayer · 2016-03-14T23:11:21Z

@CLowbrow could unstyled mean divs ? I would imagine Divs to be the most generic block container.

mitermayer · 2016-03-14T23:12:07Z

src/model/paste/__tests__/DraftPasteProcessor-test.js

@@ -130,8 +130,8 @@ describe('DraftPasteProcessor', function() {
  });



Updated 'p' to become 'div' since we were using 'p' with the assumption that would default to unstyled

sophiebits · 2016-03-14T23:18:24Z

p tags tend to be pretty annoying since browsers don't let you put any block-level tags (ex: div) inside of them. I've sworn off using <p> tags whenever I have a choice (opting instead for <div class="paragraph"> or similar) and recommend others do too. Is there a reason that they're necessary here?

sophiebits · 2016-03-14T23:27:41Z

(I guess we already have the reverse mapping though?)

mitermayer · 2016-03-14T23:31:48Z

@spicy, I agree to some extent to that. Paragraphs can definitely be an annoyance. However they provide semantics and also makes a lot easier on moving to draftjs from existing applications. Another option would be to make this behaviour configurable. We could default to paragraphs being unstyled but however allow a configuration option to overwrite it to include them as a valid block type.

CLowbrow · 2016-03-14T23:37:51Z

The current behavior is a little complicated. We basically ignore divs in the html structure unless there are no semantic elements. HTML will often contain something like <div><blockquote> hi </blockquote></div> and since we are flattening the tree we want to use the most important element in that case.

We apply unstyled tags to the content of <p> tags and to blocks that don't have any of the tags we look for. For example <h1> hello </h1> text <h2> goodbye</h2> and <h1> hello </h1> text <h2> goodbye</h2> and <h1> hello </h1><p> text <p><h2> goodbye</h2> and <div><h1> hello </h1><div> text <h2> goodbye</h2> will all generate the same block structure.

Since, generally, we don't want to treat divs as meaningful tags, I don't think we need two types that mean text block.

I might be missing something though. Can you provide an example of the case you are trying to solve for?

mitermayer · 2016-03-15T02:18:02Z

One example is that if you currently copy and paste unsupported blocks it by default get's rendered as unstyled (which is currently used as paragraphs), which makes harder to export documents where we want to be able to correctly identify which elements where meant to be real paragraphs and which elements where just unsupported block elements.

CLowbrow · 2016-03-15T02:22:50Z

That makes sense.

@hellendag I think this is taking us back to the "pass a map of block tag names to types" thing. I think the best solution is for this to be configurable with minimal defaults. Thoughts?

mitermayer · 2016-03-15T02:24:49Z

I like the idea of being configurable, happy to amend the PR if you guys reckon that's the way to go

hellendag · 2016-03-15T15:19:51Z

@hellendag stuff broke with some renderers when we added the H4/5/6 stuff right? I'm worried the same stuff is going to happen here. Do our renderers know how to deal with this type? I'm half-remembering all of this, though.

Yeah, they did. It was my fault though, I didn't sanitize it properly. paragraph is a bit more worrying to me, though, since we really just don't even use that type.

@hellendag I think this is taking us back to the "pass a map of block tag names to types" thing. I think the best solution is for this to be configurable with minimal defaults. Thoughts?

Yep. I think this is the right thing to do. Configurable block mapping, inline style mapping, and link extraction.

mitermayer · 2016-03-15T23:25:36Z

@hellendag @CLowbrow, been thinking through and I really see the benefits of having block mapping, inline styles and link extractions as a configurable configuration.

Would allow consumers to create their custom block maps and would allow draft-js to not have to maintain new blocks and styles, would help on avoiding library bloating.
Would also make upgrades a lot less risky, since there would be no surprises, as each consumer would know exactly which styles and block elements they support based on their configuration setting.

In order for this to work, I would suggest that configurations should be explicit overwrites, this would allow us to either have a large default configuration on draft-js or just a very simple basic one.

My recommendation is keeping draft-js defaults for block mapping, inline style mapping very basic and instead move most of the advanced blocks and inline styles as configuration to the examples.

What are your thoughts on it ?

hellendag · 2016-03-16T00:49:26Z

I definitely agree. Would you like to give it a try for this PR?

My recommendation is keeping draft-js defaults for block mapping, inline style mapping very basic and instead move most of the advanced blocks and inline styles as configuration to the examples.

Yes. A sensible default is important and I think it makes sense to keep our existing mapping as that default.

mitermayer · 2016-03-16T03:30:48Z

Sounds good, will amend this PR. The scope will be to make the current:

Make it configurable settings
Make the current settings the default settings
Add tests and update documentation
Update an example or create a new one to use configuration settings.

mitermayer · 2016-03-23T22:34:03Z

Just commenting in here to inform that this PR is not dead yet, working on the above decision. Since it involves major internal changes is taking a little longer, But will update this ASAP

- Adding new DraftBlockMap type - Making blocks extensible - Exposing default blocks so that it can be used to be extended - Updating the rich example to use custom blocks by adding a paragraph - creating a new customBLockMap type - updating getElementForBlockType.js and getWrapperTemplateForBlockType to accept an optional DraftBlockMap to use intead of default - updating DraftPasteProcessor to accept custom blockmaps - updating tests - adding dev gulp task to speed up development when testing using one of the examples

mitermayer · 2016-03-28T00:20:49Z

examples/rich/rich.html

@@ -89,6 +89,17 @@
            }
          }

+          var customBlockMap = Object.assign({}, DefaultDraftBlock, {


Should I update this to use Immutable ?

There is an issue open for this: #52. @sstur is going to take a crack at it.

Edit: Oh wait, I should read the changeset more thoroughly. That issue applies to inline styles.

Should I leave as it is then ?

hellendag · 2016-04-04T02:30:12Z

src/model/encoding/convertFromHTMLToContentBlocks.js

-        lastLastBlock === 'br' &&
-        (!inBlock || getBlockTypeForTag(inBlock, lastList) === 'unstyled')
+      lastLastBlock === 'br' && (!inBlock ||
+      getBlockTypeForTag(inBlock, lastList, draftBlockRenderMap) === 'unstyled')


Spacing nit, just a personal preference really:

if ( lastLastBlock === 'br' && ( !inBlock || getBlockTypeForTag(...) === 'unstyled' ) ) {

I don't mind it, we should probably add this as a rule to eslintrc so that the formatting can be picked up on the linting

hellendag · 2016-04-04T02:31:19Z

Nice! I think we're really close. :)

- Removed modules getElementForBlockType and getWrapperTemplateForBlockType in favour of inlining behaviour - updated convertFromHTMLToContentBlocks.js to use getIn and also check for the DefaultBLockRender unstyled element in case none is provided since we apply them in that case - Fixed flow Error suppressing comment. Unused suppression - Fixed indent

- Updated the example file to use state and avoid multiple object creations during render

- Updating to use Immutable.getIn and removing extranous checks

mitermayer · 2016-04-04T09:02:09Z

src/model/encoding/convertFromHTMLToContentBlocks.js

-  var workingBlocks = containsSemanticBlockMarkup(html) ? blockTags : ['div'];
+  var workingBlocks = containsSemanticBlockMarkup(html, supportedBlockTags) ?
+    supportedBlockTags :
+    ['div'];


I wonder if the intention here would be to default to unstyled if so will also update it here

will leave as it is for this PR to avoid any further change since it is already big enough

hellendag · 2016-04-05T21:58:21Z

Ok, let's do this. :)

hellendag · 2016-04-05T21:58:29Z

@facebook-github-bot import

mitermayer · 2016-04-05T23:30:18Z

awesome!

facebook-github-bot · 2016-04-06T22:03:05Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

bradleyayers · 2016-04-21T06:36:22Z

src/Draft.js

@@ -11,6 +11,8 @@

 'use strict';

+const DefaultDraftBlockRenderMap = require('DefaultDraftBlockRenderMap');


FYI the rest of the imports are alphabetical order.

hellendag · 2016-04-24T22:13:17Z

Sorry for the long wait. :) This is in review internally, and we're being thorough to make sure it works cleanly.

hellendag · 2016-05-01T01:26:36Z

Okay, I'm going to land and merge this today.

I've made quite a few changes, so let me know if you notice anything missing or incorrect.

jpuri · 2016-05-04T12:16:05Z

Hello @mitermayer : I am working on creating an editor using DraftJS and we were looking for something similar in supporting paragraph blocks. Thanks a lot for good work 👍

Can you please guild me where can I get documentation for passing configuration parameters to the editor. I could not find that in the commit.

mitermayer · 2016-05-04T23:57:00Z

@jpuri Sorry for not including the documentation on this PR it will be bundled on a separate PR with example.

In the meantime you can follow the reverse of this commit, 575e051 this had an example on how to integrate paragraph to the rich.html demo and also how to overwritte the default 'unstyled' to become a paragraph.

jpuri · 2016-05-12T06:30:22Z

Thanks a lot @mitermayer, but editor gives me this error with the configurations, should I just ignore it ?

mitermayer · 2016-05-12T06:50:07Z

@jpuri Will talk via slack in order to reduce feedback loop and help debug whats the problem

jpuri · 2016-05-12T07:55:19Z

Thanks @mitermayer

mitermayer · 2016-05-12T12:05:27Z

Attached the article documentation for it on the PR #387

Summary: This fixes issue facebookarchive#204 - Including paragraph on converter - Updating website and demo - Updating example Closes facebookarchive#205 Reviewed By: ezequiel Differential Revision: D3147267 fb-gh-sync-id: ae51df9b838597066a699dced879b2795e59db26 fbshipit-source-id: ae51df9b838597066a699dced879b2795e59db26

Summary: This fixes issue facebookarchive/draft-js#204 - Including paragraph on converter - Updating website and demo - Updating example Closes facebookarchive/draft-js#205 Reviewed By: ezequiel Differential Revision: D3147267 fb-gh-sync-id: ae51df9b838597066a699dced879b2795e59db26 fbshipit-source-id: ae51df9b838597066a699dced879b2795e59db26

adding-support-for-paragraph - Adding paragraph to demo and to conver…

280a0fe

…t module - Including paragraph on converter - Updating website and demo - Updating example

mitermayer mentioned this pull request Mar 14, 2016

DraftBlockType has 'paragraph' as a type, however getElementForBlockType does not have paragraph reference #204

Closed

mitermayer reviewed Mar 14, 2016
View reviewed changes

facebook-github-bot added the CLA Signed label Mar 14, 2016

adding-support-for-paragraph - Removing extra headers from rich.html …

286fa89

…since it has been added on another pull request - Removed the inclusion of the extra headers from the rich.html since a new PR is dealing with it #207

adding-support-for-paragraph - Fixing tests

a442439

- When we used paragraph as an invalid element to generate unstyled swapped that to use div instead

mitermayer reviewed Mar 14, 2016
View reviewed changes

hellendag mentioned this pull request Mar 16, 2016

missing-headers-on-convertFromHTMLToContentBlocks - Including missing… #207

Closed

mitermayer reviewed Mar 28, 2016
View reviewed changes

hellendag reviewed Apr 4, 2016
View reviewed changes

hellendag mentioned this pull request Apr 4, 2016

How should convertFromHTMLToContentBlocks process media tags ? #260

Closed

mitermayer added 3 commits April 4, 2016 14:07

adding-support-for-paragraph - updating example file

e06c38a

- Updated the example file to use state and avoid multiple object creations during render

adding-support-for-paragraph - Cosmetic changes

8ff93ac

- Updating to use Immutable.getIn and removing extranous checks

mitermayer reviewed Apr 4, 2016
View reviewed changes

hellendag added the PR imported label Apr 7, 2016

mitermayer mentioned this pull request Apr 8, 2016

Support custom inline styles when importing from HTML? #282

Open

bradleyayers reviewed Apr 21, 2016
View reviewed changes

rnicholus mentioned this pull request Apr 25, 2016

Recommended way to create styles on-the-fly? #52

Closed

mattmontgomery mentioned this pull request Apr 27, 2016

Add support for h4, h5, h6 block tags #345

Closed

ghost closed this in e203798 May 1, 2016

CLowbrow mentioned this pull request Jul 29, 2016

Add p tags to block render map #571

Closed

This pull request was closed.

		@@ -130,8 +130,8 @@ describe('DraftPasteProcessor', function() {
		});

		@@ -11,6 +11,8 @@

		'use strict';

		const DefaultDraftBlockRenderMap = require('DefaultDraftBlockRenderMap');

adding-support-for-paragraph - making blockTypes configurable #205

adding-support-for-paragraph - making blockTypes configurable #205

Conversation

mitermayer commented Mar 14, 2016

Choose a reason for hiding this comment

hellendag commented Mar 14, 2016

mitermayer commented Mar 14, 2016

CLowbrow commented Mar 14, 2016

mitermayer commented Mar 14, 2016

Choose a reason for hiding this comment

sophiebits commented Mar 14, 2016

sophiebits commented Mar 14, 2016

mitermayer commented Mar 14, 2016

CLowbrow commented Mar 14, 2016

mitermayer commented Mar 15, 2016

CLowbrow commented Mar 15, 2016

mitermayer commented Mar 15, 2016

hellendag commented Mar 15, 2016

mitermayer commented Mar 15, 2016

hellendag commented Mar 16, 2016

mitermayer commented Mar 16, 2016

mitermayer commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellendag commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellendag commented Apr 5, 2016

hellendag commented Apr 5, 2016

mitermayer commented Apr 5, 2016

facebook-github-bot commented Apr 6, 2016

Choose a reason for hiding this comment

hellendag commented Apr 24, 2016

hellendag commented May 1, 2016

jpuri commented May 4, 2016

mitermayer commented May 4, 2016

jpuri commented May 12, 2016

mitermayer commented May 12, 2016

jpuri commented May 12, 2016

mitermayer commented May 12, 2016