-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[wip] Preserve list indentation from one DraftEditor to another on paste #1605
[wip] Preserve list indentation from one DraftEditor to another on paste #1605
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good IF we add the conditional I mentioned.
Thanks for fixing!!!
}); | ||
}); | ||
|
||
test('Should import recognised draft li depths when nesting enabled', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost right - but I have one concern.
With "nesting enabled", the new features should create a block map where the "depth" property is not used, and instead we have true nesting set up using 'children' and 'parent' properties on the blocks.
See https://github.com/facebook/draft-js/blob/master/src/model/encoding/__tests__/__snapshots__/convertFromHTMLToContentBlocks-test.js.snap#L303 for an example.
It looks like with this change, we default to using the 'depth' property again for nesting, even with the 'nesting' flag enabled, when those classes are present.
I think we should add a conditional for your check that sets the 'depth' and disable it when the nesting flag is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - thanks for the pointer. So in the instance that nesting
is true
should there be an alternative way to handle converting the li indentation?
[cx('public/DraftStyleDefault/depth2')]: 2, | ||
[cx('public/DraftStyleDefault/depth3')]: 3, | ||
[cx('public/DraftStyleDefault/depth4')]: 4, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, list indentations above 4 would not be preserved. Could this be changed to support arbitrary depth?
I imagine the current approach is based on the class names defined in https://github.com/facebook/draft-js/blob/232791a4e92d94a52c869f853f9869367bdabdac/src/component/contents/DraftEditorContents-core.react.js#L58-L62. An alternative would be to generate those class names dynamically with the right depth suffix in getListItemClasses
, and here in getListItemDepth
get the depth by looking for a number right after the cx('public/DraftStyleDefault/depth')
string.
I realise this is a bit more work but it would be nice to support all use cases of depth. While the getListItemClasses
code does stop at depth 4 at the moment, this is easily extensible via blockStyleFn
, whereas the conversion from HTML isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @thibaudcolas -
We are working on replacing this approach of using CSS classes with true nested blocks, in a longer term project. So for now we're not adding improvements to the old CSS code, but ultimately will be able to support arbitrary nesting. That is indeed a good idea. :)
I would not block this diff on that change either way.
When pasting from one editor to another we lose list indentation. By inspecting the classList of each list item contained in HTML pasted we can preserve this state
e49c4d5
to
700562a
Compare
…rrently be honored between pastes From my understanding this support will be implemented as experimentalTreeDataSupport feature is built out
Created an internal task to have someone enable this in tree-data-structure mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flarnie is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: **Summary** When pasting from one editor to another we lose list indentation. By inspecting the classList of each list item contained in HTML pasted we can preserve this state. **Test Plan** I validated behavior manually: - Rebuild - Open two tabs to rich text example - In one create indented list - Select all - Paste into other - Styling is preserved I also added some snapshot testing for regression testing. Closes facebookarchive/draft-js#1605 Reviewed By: flarnie Differential Revision: D6702583 Pulled By: flarnie fbshipit-source-id: 071cecc786e2aaef11af9bd21cc03f9b2544a2b4
Summary: **Summary** When pasting from one editor to another we lose list indentation. By inspecting the classList of each list item contained in HTML pasted we can preserve this state. **Test Plan** I validated behavior manually: - Rebuild - Open two tabs to rich text example - In one create indented list - Select all - Paste into other - Styling is preserved I also added some snapshot testing for regression testing. Closes facebookarchive/draft-js#1605 Reviewed By: flarnie Differential Revision: D6702583 Pulled By: flarnie fbshipit-source-id: 071cecc786e2aaef11af9bd21cc03f9b2544a2b4
Summary
When pasting from one editor to another we lose list indentation. By inspecting the classList of each list item contained in HTML pasted we can preserve this state.
Test Plan
I validated behavior manually:
I also added some snapshot testing for regression testing.