Exploration - Feature tree / Rendering tree data format #1508

mitermayer · 2017-11-13T23:43:29Z

Context

This PR is part of a series of PR's that will be exploring tree data block support in Draft.

Rendering tree data format

This PR adds support for react components to render the tree data structure

Note:
This is unstable and not part of the public API and should not be used by
production systems.

mitermayer · 2017-11-13T23:46:51Z

src/component/contents/DraftEditorContents.react.js

-          className,
-          getListItemClasses(blockType, depth, shouldResetCount, direction),
-        );
+    if (rootBlock && rootBlock instanceof ContentBlockNode) {


For now left the rendering logic for tree inside DraftEditorContents.react.js, but when we introduce a new feature flag to control this logic we can move this out to a tree aware variant of DraftEditorContents.react.js and use the feature flag to chose between both.

Decided to integrate in here for now to make it easier to review and get early end to end progression

mitermayer · 2017-11-13T23:48:33Z

src/component/contents/exploration/__tests__/DraftEditorContents.react-test.js

@@ -0,0 +1,142 @@
+/**


We have a file called DraftEditorContent.react-test.js however since I plan to move the logic for rendering tree outside of that file it made sense to create a separate test file as opposed to add to the existing one.

We talked about this and agreed that it might be nice to rename this - for now could we rename the other one to DraftEditorContents.react-test and call this one DraftEditorContentsExperimental.react-test or something like that?

mitermayer · 2017-11-13T23:55:00Z

src/component/contents/DraftEditorContents.react.js

+          tree: editorState.getBlockTree(blockKey),
+        };
+
+        const configForType =


Due to DraftEditorBlockNode only handle wrapping for its own children had to still rely on the augment functionality provided by this file for wraping the rootBlock nodes (https://github.com/facebook/draft-js/pull/1508/files#diff-42e035f607d4b34ff0d0592ee0b49266R286)

Once we introduce featutre control flag move this logic out of this file we can then avoid this

flarnie

still in progress reviewing

flarnie · 2017-11-14T15:15:30Z

src/component/contents/DraftEditorBlock.react.js

@@ -16,6 +16,7 @@
 import type {BlockNodeRecord} from 'BlockNodeRecord';


Just confirming - looks like the only changes to this file were cosmetic, improving and modernizing the style and organization.

Thats correct! only cosmetic chanes

flarnie · 2017-11-14T15:35:11Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+ * This is unstable and not part of the public API and should not be used by
+ * production systems. This file may be update/removed without notice.
+ */
+


Can we leave a comment explaining that this is a fork of DraftEditorBlock? And we can remove the comment once we no longer have both versions.

We should do the same for BlockNode.js, saying that it's a fork of ContentBlock which adds nodes, parent, prevSibling, and nextSibling.

flarnie · 2017-11-14T15:38:13Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+
+const {List} = Immutable;
+
+type BlockRenderFn = (block: BlockNodeRecord) => ?Object;


Should the return type be something like React.Node?

I got this from the main file https://github.com/facebook/draft-js/blob/master/src/component/base/DraftEditorProps.js#L183

I suggest we update all of that later, could even raise this as a good first bug, updating the flow definitions

flarnie · 2017-11-14T15:47:59Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+  forceSelection: boolean,
+  selection: SelectionState,
+  startIndent?: boolean,
+  tree: List<any>,


This was already named tree in the previous one, just wondering if we can make the name more descriptive while we are here. Or open an issue to requests someone do so, here and in DraftEditorBlock.

I agree, do you have some suggestions ?

flarnie · 2017-11-14T15:49:23Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+type BlockRenderFn = (block: BlockNodeRecord) => ?Object;
+type BlockStyleFn = (block: BlockNodeRecord) => string;
+
+type Props = {


For my own notes - this adds the following props which are not passed into DraftEditorBlock:

blockRenderMap

blockRenderFn

contentState

editorKey

editorState

flarnie · 2017-11-14T16:10:47Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+const getDraftRenderConfig = (
+  block: BlockNodeRecord,
+  blockRenderMap: DraftBlockRenderMap,
+): * => {


We could at least say it returns type Object and leave a TODO to add a specific type. Same below with getCustomRenderConfig.

Sounds good to me, I was leaning towards adding typing now but the PR was already too big, happy to do this on follow up PR's

flarnie · 2017-11-14T16:14:07Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+  editorKey: string,
+  offsetKey: string,
+  blockStyleFn: BlockStyleFn,
+  customConfig: *,


if this customConfig param is the same as the return value of getCustomRenderConfig we could add a specific type and reuse it. Could be done in a follow-up commit.

flarnie · 2017-11-14T16:18:01Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+    const {block, direction, tree} = this.props;
+
+    return (
+      block.getChildKeys() !== List() ||


Just noting that we talked about updating this conditional.

That's correct this conditional logic as it is is wrong, will update it

flarnie · 2017-11-14T16:30:43Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+        );
+        const childProps = {
+          ...this.props,
+          tree: editorState.getBlockTree(key),


I hate to pass the editorState through in order to use it just once. Not sure how to extract what we need here though.

I could pass just the editorState.getBlockTree method reference, what do you reckon ?

Sounds good - passing a bound function like that is a bit unconventional, but I can't think of any strong reason not to.

flarnie · 2017-11-14T16:49:06Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+
+        return acc;
+      }, []);
+    }


This might be nice as a helper function, like this._renderChildren.

for rendering tree data structure - This PR adds support for rendering tree data structure

SirensOfTitan

First pass: I haven't quite wrapped my head around the larger idea yet, but I'm getting there.

SirensOfTitan · 2017-11-14T16:54:35Z

src/component/contents/DraftEditorBlock.react.js

  block: BlockNodeRecord,
+  blockProps?: Object,


Can we type this a bit better? Perhaps as something like: {+[key: string]: *} to force covariance. * may not work either so any would work too.

This is already existent draft editor block, we definitely should improve typing but this should probably come up on a different PR

I agree that we could improve the types - seems like a good thing to work on as a follow up. We can make a list and either bootcamp or share with @juliankrispel .

We have run into some type definitions that pass CI on Github but break internally, so we are also being careful so we don't get blocked by that.

SirensOfTitan · 2017-11-14T17:19:05Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+
+  // we check back until we find a sibbling that does not have same wrapper
+  for (const sibling: any of nodes.reverse()) {
+    if (sibling.type !== Element) {


Why are we tagging this block as any? React.Node could be null or a number, amongst other things, so we should insulate against that?

+nit, but since this is so simple why don't we avoid a break?

for (const sibling of nodes.reverse()) { if (sibling && sibling.type === Element) { wrappedSiblings.push(sibling); } } // or this could be reduced down to a simple filter: const wrappedSiblings = nodes.reverse().filter(node ==> node && node.type === Element);

We want to avoid unnecessary iterations on larger documents

SirensOfTitan · 2017-11-14T17:20:51Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+  blockStyleFn: BlockStyleFn,
+  customConfig: *,
+): Object => {
+  let elementProps: Object = {


Again, I don't know what flow internals are around props, but using a CovariantObject is safer, so perhaps getElementPropsConfig should return the type {+[key: string]: any};

Been trying to play safe on this PR since I don't want to get blocked by flow types, a lot of times flow internal is behaving different than here so I think we can create a follow up PR / issue to harden up draft.js typings in general

SirensOfTitan · 2017-11-14T17:22:33Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+    const {block, direction, tree} = this.props;
+
+    return (
+      block.getChildKeys() !== List() ||


!block.getChildKeys.isEmpty() as perhaps something more readable?

SirensOfTitan · 2017-11-14T17:23:14Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+      block !== nextProps.block ||
+      tree !== nextProps.tree ||
+      direction !== nextProps.direction ||
+      (isBlockOnSelectionEdge(nextProps.selection, nextProps.block.getKey()) &&


This line perhaps deserves a comment. So why would we update if we're on a selection edge and want to forceSelection?

This is legacy from the port of the other BlockNode

SirensOfTitan · 2017-11-14T17:28:00Z

src/component/contents/exploration/DraftEditorDecoratedLeaves.react.js

+    } = this.props;
+
+    const blockKey = block.getKey();
+    const leavesForLeafSet = leafSet.get('leaves');


Perhaps I'm missing something here, but leafSet is a List, which means get takes numbers. Is leafSet typed correctly?

Will double check, could be that it's typed incorrectly

flarnie

I think this is a good start. We talked through it a bit and this is my impression of the high level overview:

The logic for rendering the set of DraftEditorBlocks used to mainly live in DraftEditorContents.
A block can now be either a normal block or a container that holds other blocks, since we are adding support for ntested blocks. For that reason we can't just do the block rendering in DraftEditorContents.
A block containing other blocks must render those blocks within itself, so that is what DraftEditorBlockNode does.
We are breaking out DraftEditorNode and DraftEditorDecoratedLeaves in order to keep things modular and organized. Those contain some of the logic for rendering the DraftEditorLeaf within a DraftEditorBlockNode.
We still retain some logic for rendering the 'root' blocks inside DraftEditorContents, because the DraftEditorBlockNode cannot render itself. In follow up diffs we will fork DraftEditorCOntents and organize/improve the logic for rendering those root blocks.

Glad to see so many tests!!!

flarnie · 2017-11-14T18:28:44Z

src/component/contents/exploration/DraftEditorDecoratedLeaves.react.js

+ * This is unstable and not part of the public API and should not be used by
+ * production systems. This file may be update/removed without notice.
+ */
+


Nice - looks like this is pulled out from DraftEditorBlock to keep things modular.

flarnie · 2017-11-14T18:32:25Z

src/component/contents/exploration/DraftEditorNode.react.js

+          const decoratorKey = leafSet.get('decoratorKey');
+          const leavesForLeafSet = leafSet.get('leaves');
+          const lastLeaf = leavesForLeafSet.size - 1;
+          const Leaves = leavesForLeafSet


nit - Leaves to leaves

flarnie · 2017-11-14T18:45:17Z

src/component/contents/exploration/DraftEditorBlockNode.react.js

+
+    const blockKey = block.getKey();
+    const offsetKey = DraftOffsetKey.encode(blockKey, 0, 0);
+


So the legacy approach was to render DraftEditorLeaf inside DraftEditorBlock, like so:

DraftEditorBlock | |-> DraftEditorLeaf

Now we have added more layers, with DraftEditorBlockNode rendering DraftEditorNode which renders DraftEditorLeaf wrapped optionally in DraftEditorDecoratedLeaves:

DraftEditorBlockNode | |-> DraftEditorNode | |-> DraftEditorDecoratedLeaves | |-> DraftEditorLeaf

Is the point of this new structure to make the code more clean and modular? Since we may have many blocks on the page, I wonder if it will affect performance.

flarnie · 2017-11-14T19:10:28Z

src/component/contents/exploration/__tests__/DraftEditorBlockNode.react-test.js

+ */
+
+'use strict';
+


Just mentioning - if we can merge this it might be helpful to get the tests working - #1464

flarnie · 2017-11-14T19:23:28Z

src/component/contents/exploration/__tests__/DraftEditorBlockNode.react-test.js

+  });
+});
+
+test('renders block with nested child that if of different block type', () => {


nit - if of different -> is of different

flarnie · 2017-11-14T19:30:17Z

src/component/contents/exploration/__tests__/DraftEditorContents.react-test.js

@@ -0,0 +1,142 @@
+/**


We talked about this and agreed that it might be nice to rename this - for now could we rename the other one to DraftEditorContents.react-test and call this one DraftEditorContentsExperimental.react-test or something like that?

…om-tree-data

internally

amends - Adding comment metadata in regards to file forking - Test files semantic renaming - Clarifying logic on shouldComponentUpdate - Minor tweak and nits

facebook-github-bot

@mitermayer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: This PR is part of a series of PR's that will be exploring **tree data block support** in Draft. **Rendering tree data format** This PR adds support for react components to render the tree data structure *** **Note:** This is unstable and not part of the public API and should not be used by production systems. Closes facebookarchive/draft-js#1508 Differential Revision: D6341091 fbshipit-source-id: e285bb04ca51eb55b75c6912a2f5d283a4145d2d

facebook-github-bot added the CLA Signed label Nov 13, 2017

mitermayer requested a review from flarnie November 13, 2017 23:43

mitermayer added the exploration label Nov 13, 2017

mitermayer commented Nov 13, 2017

View reviewed changes

mitermayer force-pushed the feature-tree/rendering-react-components-from-tree-data branch from d6b21b0 to e8b60b9 Compare November 14, 2017 03:58

flarnie reviewed Nov 14, 2017

View reviewed changes

feature-tree/rendering-react-components-from-tree-data - adding support

32edac7

for rendering tree data structure - This PR adds support for rendering tree data structure

SirensOfTitan reviewed Nov 14, 2017

View reviewed changes

flarnie approved these changes Nov 14, 2017

View reviewed changes

mitermayer added 4 commits November 15, 2017 12:10

Merge branch 'master' into feature-tree/rendering-react-components-fr…

dd8a6d0

…om-tree-data

feature-tree/rendering-react-components-from-tree-data - rebasing

4bffd02

Adding test helper to allow snapshot compatibility when running tests

52ab067

internally

feature-tree/rendering-react-components-from-tree-data - tech review

8de180a

amends - Adding comment metadata in regards to file forking - Test files semantic renaming - Clarifying logic on shouldComponentUpdate - Minor tweak and nits

mitermayer force-pushed the feature-tree/rendering-react-components-from-tree-data branch from e8b60b9 to 8de180a Compare November 15, 2017 22:10

facebook-github-bot reviewed Nov 15, 2017

View reviewed changes

facebook-github-bot closed this in c1150a7 Nov 16, 2017

		@@ -16,6 +16,7 @@
		import type {BlockNodeRecord} from 'BlockNodeRecord';


		const {List} = Immutable;

		type BlockRenderFn = (block: BlockNodeRecord) => ?Object;


		const blockKey = block.getKey();
		const offsetKey = DraftOffsetKey.encode(blockKey, 0, 0);

		*/

		'use strict';

Exploration - Feature tree / Rendering tree data format #1508

Exploration - Feature tree / Rendering tree data format #1508

Conversation

mitermayer commented Nov 13, 2017

Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitermayer Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

flarnie left a 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

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

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

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

SirensOfTitan left a 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

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

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

flarnie left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

mitermayer Nov 13, 2017 •

edited

Loading