[lexical-list][lexical-markdown] Feature: Keep list marker for markdown round-trip#7892
[lexical-list][lexical-markdown] Feature: Keep list marker for markdown round-trip#7892etrepum merged 15 commits intofacebook:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I don't think this really belongs in ListItemNode, lexical is not really a markdown native editor so it doesn't make sense to always try and store markdown related things even when you are not using it. One thing you could do is to write custom markdown transformers that use NodeState to attach this metadata to the nodes it creates so that they can be used on markdown serialization (and that data would end up in the JSON for those nodes as well). https://lexical.dev/docs/concepts/node-state The issues you had writing tests probably had something to do with how the editors are configured in the specific places you were trying to write tests. There is no check list transformer in the markdown package, but there is one in the playground, so you'd have to copy that into the tests to use it. A primary reason for that is that such a transformer would fail if the check list plugin is not installed. Extensions will solve this once someone implements a markdown extension, but that's why it only ships with default transformers that are likely to be enabled in a basic rich text editor. |
Signed-off-by: Simon Bauchet <simon@vivaldi.com>
| const TABLE_ROW_REG_EXP = /^(?:\|)(.+)(?:\|)\s?$/; | ||
| const TABLE_ROW_DIVIDER_REG_EXP = /^(\| ?:?-*:? ?)+\|\s?$/; | ||
|
|
||
| export const listMarkerState = createState('listMarker', { |
There was a problem hiding this comment.
might be worth coming up with a more globally unique name for this, since it does only apply to markdown and it's not allowed for two different states on the same node to have the same name
There was a problem hiding this comment.
What about mdListMarkerState ?
There was a problem hiding this comment.
mdListMarker seems reasonable, "state" is redundant
| ); | ||
| if (listType === 'bullet' || listType === 'check') { | ||
| const marker = match[0].trim()[0]; | ||
| $setState(listItem, listMarkerState, marker); |
There was a problem hiding this comment.
This state should really be on the list rather than the list item since it's not really permitted to mix different markers in the same unordered list. It's also a lot more compact as far as serialization goes.
if you put it on the ListItemNode what will happen is that you deserialize from markdown, then edit in lexical, you will end up with your custom markers and then any new items will be the default marker. You'll also run into the same sort of issues when copy and paste because the list items might have been created with different markers than their siblings.
Would be a good idea to have some test coverage that checks that the lists have consistent markers after such operations
Signed-off-by: Simon Bauchet <simon@vivaldi.com>
- marker is kept when using indented list (indented list should have default marker) - marker is kept when copy/pasting list Signed-off-by: Simon Bauchet <simon@vivaldi.com>
| const TABLE_ROW_REG_EXP = /^(?:\|)(.+)(?:\|)\s?$/; | ||
| const TABLE_ROW_DIVIDER_REG_EXP = /^(\| ?:?-*:? ?)+\|\s?$/; | ||
|
|
||
| export const listMarkerState = createState('listMarker', { |
There was a problem hiding this comment.
mdListMarker seems reasonable, "state" is redundant
etrepum
left a comment
There was a problem hiding this comment.
This all seemed to work fairly well in some brief testing in the playground, and the additional unit and e2e tests are nice
|
I think this is good to merge but could you fix up the PR description to match the current state of the PR (e.g. there are tests now)? This will help when putting together the release notes. |
|
Done, I refactored the Unit Test part by:
And added a small description of what the PR does |
|
It seems I'm missing something to merge it, I don't have the merge button. |
|
The merge button is only there for maintainers |
Description
I'm submitting a proposal so list markers are not loss anymore when switching from/to markdown.
This is a continuation in the idea that switching from/to markdown should not modify typed text.
These changes add a state to NodeList that save list marker between markdown import/export.
Test plan
In lexical playground
In view mode:
* helloBefore
-> markdown is
- helloAfter
-> markdown is
* hello, it kept markerSwitching again and again keep marker.
Unit tests
Added unit tests in
LexicalMarkdown.test.tsthat verify a marker is kept between import/exportAdded e2e tests in
Markdown.spec.mjsthat verify markers are kept when typing in lexical view and when copy/pasting.