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

Tree block structure #388

Closed
wants to merge 25 commits into from
Closed

Conversation

SamyPesse
Copy link

@SamyPesse 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
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
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
Copy link
Author

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
Copy link
Contributor

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

@SamyPesse
Copy link
Author

@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,
Copy link
Contributor

Choose a reason for hiding this comment

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

are we using this anywhere in here ?

@mitermayer
Copy link
Contributor

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 feature/tree branch 3 times, most recently from adfe43a to 3cd2638 Compare May 13, 2016 07:08
@SamyPesse
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Contributor

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

@mitermayer
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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 feature/tree branch 2 times, most recently from 3b8411f to cbe4975 Compare May 19, 2016 03:53
@mitermayer mitermayer force-pushed the feature/tree branch 2 times, most recently from 7da6cd4 to 971c8a7 Compare May 20, 2016 06:48
@rafalp
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
Copy link

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

@SamyPesse
Copy link
Author

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

mitermayer/draft-js already exists

@rafalp
Copy link

rafalp commented Oct 5, 2016

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

@mitermayer
Copy link
Contributor

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

@SamyPesse
Copy link
Author

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

screen shot 2016-10-06 at 09 40 10

@mitermayer
Copy link
Contributor

@SamyPesse done, just deleted it

@SamyPesse
Copy link
Author

Done 🍻 Good luck with this PR !

@mitermayer
Copy link
Contributor

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
Copy link
Contributor

mitermayer commented Oct 24, 2016

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

@flarnie
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
Copy link
Contributor

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

@flarnie
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
Copy link
Contributor

@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 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.