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

Tree block structure #388

Closed
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
@SamyPesse
Copy link

SamyPesse commented May 12, 2016

This PR implements support for a tree structure that can be used to render tables, blockquote, list, etc. For context on this topic, see discussion in #143.

The main goal is also to not break compatibility with people already using Draft, and keep “flatness” as the basic use case.

Implementation

To represent a tree and keep compatibility with the flat model, we decide to not change the data structure, but instead to leverage keys in the ContentState.blockMap.

Basically, children of the block a will be blocks in the blockMap with keys a/b, a/c; and a/b/d is a child of a/b.

Demo

A demo is available at samypesse.github.io/test-draft-js/ (similar to the tree example).

Compatibility

Nesting is disabled by default to keep “flatness” as the basic use case. It should be enabled for the right block types in the blockRenderMap, and Draft will provides a NestedUtils module to help use nested blocks and provide the right editing UX.

@ghost

This comment has been minimized.

Copy link

ghost commented May 12, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost

This comment has been minimized.

Copy link

ghost commented May 12, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label May 12, 2016

@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented May 12, 2016

I'm seeing potential implementations:

1) Block can have children using blockMap

The problem with this implementation is: modifier need to resolve in the tree a blockKey, edit it, then set it in the tree.

2) ContentState keeps a flatten blockMap but blockKey can contain slashes

For example: 8plur is a first level block, and 8plur/7j4ue is a block contained by 8plur.

convertFromRaw can take care of creating this flatten map from a a real tree object.

With this implementation, modifiers will not require much changes.

Current state of mind:

I've started implementing 1), but I'm thinking that 2) will be easier to implement and maintain.

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented May 12, 2016

Hi @SamyPesse checking out your branch now. Are you on the slack draft-js channel may be easier to communicate through there?

@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented May 12, 2016

@mitermayer Yes, I'm on the slack channel, but I'm going to stop for today (it's 2am in France).

I'd love to get your input on the branch and the beginning of the implementation, you can test using the tree example.

Basically, the first rendering and basic edition are working, but blocks are not correctly refresh (for example when pressing Enter and splitting a block) since the parent block prevents rendering in the shouldComponentUpdate


var {
List,
OrderedSet,
OrderedMap,

This comment has been minimized.

@mitermayer

mitermayer May 13, 2016

Member

are we using this anywhere in here ?

@@ -56,6 +56,17 @@ class ContentState extends ContentStateRecord {
return block;
}

getFirstLevelBlocks(): BlockMap {
return this.getBlockChildren('');

This comment has been minimized.

@mitermayer

mitermayer May 13, 2016

Member

Should we instead create a better way to define the root component? maybe a pre-defined key such as 'root' or something that could add a bit more meaning ?

This comment has been minimized.

@SamyPesse

SamyPesse May 13, 2016

It may break compatibility if we introduce a root block with a special key.

I'm not sure how people using Draft store the contentState between editing sessions, but I think we should have as goal: "The exact data set from Draft '0.7.0' should work without changes in this version".

This comment has been minimized.

@mitermayer

mitermayer May 13, 2016

Member

Agreed, lets keep as it is

@@ -204,6 +213,7 @@ class DraftEditorBlock extends React.Component {
return (
<div data-offset-key={offsetKey} className={className}>
{this._renderChildren()}

This comment has been minimized.

@mitermayer

mitermayer May 13, 2016

Member

do we still need to _renderChildren(), or can we do all through _renderBlockMap() ?

This comment has been minimized.

@SamyPesse

SamyPesse May 13, 2016

We still need _renderChildren() to render the inner text (with styles and entities).

_renderBlockMap() can only render the children blocks.

I think we should keep it this way: block should still have text, styleRanges and entitieRanges, even when they have a blockMap, it will ensure compatibility with previous versions of Draft (only extend it).

This comment has been minimized.

@mitermayer

mitermayer May 13, 2016

Member

Sounds good to me. Maybe we should even abstract both of them into a single facade. We could name the facade _renderChildren, and rename the current '_renderChildren' to something else. This way we could do the logic of checking if a blockType is a valid container in there.

* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule DraftEditorContents.react

This comment has been minimized.

@mitermayer

mitermayer May 13, 2016

Member

this should be

@providesModule  DraftEditorBlocks.react
@@ -0,0 +1,210 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.

This comment has been minimized.

@mitermayer

mitermayer May 13, 2016

Member

I am not sure if we need this... if each DraftEditorBlock know that it has children, and if it has a parent, there is no need for such grouping.

This comment has been minimized.

@SamyPesse

SamyPesse May 13, 2016

I'm not sure to see how you want to implement this without rendering the inner blocks of a block.

What do you suggest?

This comment has been minimized.

@mitermayer

mitermayer May 13, 2016

Member

You are right I think we should keep on with the current approach. We can always revisit it later if needed.

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented May 13, 2016

I really liked the idea of keep the data model the same, but creating the sense of nesting via the block key!

@mitermayer mitermayer force-pushed the mitermayer:feature/tree branch 3 times, most recently from adfe43a to 3cd2638 May 13, 2016

@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented May 13, 2016

@mitermayer I'm happy to work on another implementation if the nesting via block key doesn't sound great.

Thanks for the changes and comments.

I've added a section in the PR introduction about the Rich text editing with nested blocks.

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented May 13, 2016

Should we also consider a default set of rules ? Or leave that for the application layer ? Example:

Should we support nesting like this ?

H1 > H2

Or should we have a list of blockTypes that can support nesting ? or someway to define that they can be containers ?

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented May 13, 2016

We could add an optinal property on the bockRenderMap, that could define if a block is a valid container

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented May 13, 2016

Was wondering were would be the best way to added known issues/tasks to this issue so that we can keep track of them ?

known issues / remaining task

  • [bug] changing blocktype of a parent element changes the blockType of all children
  • [feature] we need to update the convertToHTML to support the new data model
  • [feature] we need to convert convertToRaw to support data model
  • [feature] we need a way to define if the block is a valid container, we can do that on the blockRenderMap
  • [feature] we need to come up with a way to add children to existing blocks, at the moment we are using raw data. How should we do that ? create a new modifier ?
  • [feature] we need to have an example
  • [feature] we should write some tests to guarantee backwards compatibility
  • [feature] we need to fix the ci builds
@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented May 14, 2016

Should we also consider a default set of rules ? Or leave that for the application layer ?

I was thinking handling all of this into a NestedUtil, so that devs can choose to use it or to build their own rules.

Or should we have a list of blockTypes that can support nesting ? or someway to define that they can be containers ?

I agree, I'm thinking about unordered-list-item, ordered-list-item, blockquote. table-cell.

We could add an optinal property on the bockRenderMap, that could define if a block is a valid container

I'm not sure I understand, what do you mean by valid container ?
Maybe the bockRenderMap can contain a boolean to signal that text should be rendered for a block node (For Example: The text node of the table and table-row should now be rendered, only the children block).

Was wondering were would be the best way to added known issues/tasks to this issue so that we can keep track of them ?

Maybe we can use issues on the fork repository.

@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented May 16, 2016

@mitermayer I don't understand what do you mean by "valid container"

we need a way to define if the block is a valid container, we can do that on the blockRenderMap

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented May 16, 2016

@SamyPesse Sorry for taking long to answer, my son has been on the hospital, i mean by valid container a container that could have nested blocks.

Example:

  • h1 -> h2 -> would maybe be treated as an invalid block
  • li -> h1 -> could be treated as a valid block

This could be a user defined boolean on the blockRenderMap, we could chose some sensible defaults our selves.

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented May 16, 2016

Will today add a list of issues to keep track on the reamaining tasks for this issue, and will pick one of them to work on it. Will then create a new branch from your branch and start contributing via PR to your branch. this way we should be able to progress without conflicts.

@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented May 16, 2016

@mitermayer No worries, I hope your son is okay.

Got it for "valid container", but if we add it to blockRenderMap, it means that the test will be done in the React containers? I was thinking about testing this in the NestedUtil.handleKeyCommand and other modifiers.

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented May 17, 2016

@SamyPesse we could make modifiers accept a blockRenderMap as optional param. This is kinda how we are doing internally to figure out the containers to wrap content. We already do such thing for the convertFromHTMLToContentBlocks

@mitermayer mitermayer force-pushed the mitermayer:feature/tree branch 2 times, most recently from 3b8411f to cbe4975 May 19, 2016

@mitermayer mitermayer force-pushed the mitermayer:feature/tree branch 2 times, most recently from 7da6cd4 to 971c8a7 May 19, 2016

@rafalp

This comment has been minimized.

Copy link

rafalp commented Sep 30, 2016

@guangmiw8 the answer is in this very thread:

I will keep working on it from 15th of October.

Thats two weeks away from today.

@guangmiw8

This comment has been minimized.

Copy link

guangmiw8 commented Oct 1, 2016

@rafalp thank you, I was expecting a response after Samy's comment, but apparently @mitermayer has already given answer beforehand :)

@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented Oct 5, 2016

@mitermayer I can't transfer you the repository because you already have a fork:

mitermayer/draft-js already exists

@rafalp

This comment has been minimized.

Copy link

rafalp commented Oct 5, 2016

@SamyPesse could you try renaming your fork to, say, draft-js-tree-struct before transfer?

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented Oct 6, 2016

@SamyPesse Hi sammy i just renamed my fork could you please try it again? thanks a lot

@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented Oct 6, 2016

@mitermayer it looks like you have to delete your fork :/

screen shot 2016-10-06 at 09 40 10

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented Oct 6, 2016

@SamyPesse done, just deleted it

@SamyPesse

This comment has been minimized.

Copy link

SamyPesse commented Oct 6, 2016

Done 🍻 Good luck with this PR !

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented Oct 19, 2016

Hi guys, I am back working on this PR and should have an update about it pretty soon. Thank you for your time and support

@mitermayer

This comment has been minimized.

Copy link
Member

mitermayer commented Oct 24, 2016

Will start rebasing to fix conflicts with master and move this PR towards the direction of (#388 (comment))

@echenley echenley referenced this pull request Nov 7, 2016

Closed

Problems with Java syntax #10

@mitermayer mitermayer self-assigned this Dec 16, 2016

@jschr jschr referenced this pull request Dec 28, 2016

Open

Documentation? #4

@flarnie

This comment has been minimized.

Copy link
Contributor

flarnie commented Jan 20, 2017

Thanks @SamyPesse and @mitermayer and other contributors for the discussion and work on this PR!

To update the status on this: We are not going to merge this PR in the near future but we are still experimenting with it internally. A new PR will be opened in the future if we are able to make it work internally; please direct discussion to issue #143.

@flarnie flarnie closed this Jan 20, 2017

@juliankrispel

This comment has been minimized.

Copy link
Collaborator

juliankrispel commented May 24, 2017

it really is frustrating to see so much work poor into a pull request without any results. Discouraging really :/

@flarnie

This comment has been minimized.

Copy link
Contributor

flarnie commented May 30, 2017

This is a valid feeling - I apologize that we weren't able to find an owner for continuing this work. I realize I should have been more encouraging when closing this PR.

It was closed because it's not actively being worked on, and still we would be happy to work with anyone who wishes to pick up work on this PR. Feel free to take this branch and rebase it onto master and open a new PR that continues investigating this approach.

@juliankrispel

This comment has been minimized.

Copy link
Collaborator

juliankrispel commented Jun 21, 2017

@flarnie @mitermayer I'd like to contribute to moving this along, any chance I could get someone to help me walk through this implementation so I can get a headstart?

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