Skip to content
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

[lexical-list] Revert PR 6912 #6944

Merged
merged 3 commits into from
Dec 12, 2024
Merged

[lexical-list] Revert PR 6912 #6944

merged 3 commits into from
Dec 12, 2024

Conversation

potatowagon
Copy link
Contributor

Receiving reports about unexpected empty bullet lines appearing in the preview of the workplace editor

Screenshot 2024-12-11 at 10 00 33 PM

the sync diff containing this PR got reverted to mitigate the issue. Reverting this PR because:

  • more discussion is needed about list item node inheriting from paragraph node (it is odd)
  • not worth trading highly visible breaking changes for a rather edge case styling fix

Copy link

vercel bot commented Dec 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2024 7:05am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2024 7:05am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 12, 2024
Copy link

github-actions bot commented Dec 12, 2024

size-limit report 📦

Path Size
lexical - cjs 31.18 KB (0%)
lexical - esm 31 KB (0%)
@lexical/rich-text - cjs 40.15 KB (0%)
@lexical/rich-text - esm 32.83 KB (0%)
@lexical/plain-text - cjs 38.79 KB (0%)
@lexical/plain-text - esm 30.15 KB (0%)
@lexical/react - cjs 42 KB (0%)
@lexical/react - esm 34.23 KB (0%)

@etrepum
Copy link
Collaborator

etrepum commented Dec 12, 2024

Were you testing this with #6917 also applied? There's a fix in there that I believe is related to this issue

https://github.com/facebook/lexical/pull/6917/files#diff-ebf55b0878052c62d91299c29e05b3f3ef9026a05bc7881a7f0d7bb37ed2d0c9L514-R520

@etrepum
Copy link
Collaborator

etrepum commented Dec 12, 2024

As far as rationale goes, the ListItemNode is basically supposed to support everything that a ParagraphNode does. Unlike a TableCellNode, it is not a shadow root that is expected to have children that are ParagraphNode, so to support all of the things that ParagraphNode supports then it should either be a ParagraphNode or separately implement all of the things that ParagraphNode implements, but the problem is that you can't just copy and paste that because the extensibility is not present in selections and the reconciler to make that work, there are $isParagraphNode special cases that can't be extended outside of the lexical package.

If we really don't want nodes like this to subclass ParagraphNode then we should move the features that ParagraphNode has into some new common base class, e.g. LexicalNode -> ElementNode -> BlockNode -> (ParagraphNode | ListItemNode) so that this sort of behavior can be inherited and baked into the lower levels without introducing dependencies from lexical to @lexical/list or elsewhere.

Sahejkm
Sahejkm previously approved these changes Dec 12, 2024
@potatowagon
Copy link
Contributor Author

Were you testing this with #6917 also applied? There's a fix in there that I believe is related to this issue

https://github.com/facebook/lexical/pull/6917/files#diff-ebf55b0878052c62d91299c29e05b3f3ef9026a05bc7881a7f0d7bb37ed2d0c9L514-R520

thanks for pointing out #6917. the sync diff didnt contain that fix. in that case ill put this revert on hold and test with that patch

@potatowagon
Copy link
Contributor Author

potatowagon commented Dec 12, 2024

i tested with PR 6917 applied but it did not fix the issue sadly

Screenshot 2024-12-11 at 10 55 23 PM

ill proceed with this revert first to unblock the diff syncs

@potatowagon
Copy link
Contributor Author

If we really don't want nodes like this to subclass ParagraphNode then we should move the features that ParagraphNode has into some new common base class, e.g. LexicalNode -> ElementNode -> BlockNode -> (ParagraphNode | ListItemNode) so that this sort of behavior can be inherited and baked into the lower levels without introducing dependencies from lexical to @lexical/list or elsewhere.

this might work better to prevent breaking changes from $isParagraphNode(...)

@potatowagon potatowagon added this pull request to the merge queue Dec 12, 2024
Merged via the queue into main with commit 61e17c8 Dec 12, 2024
38 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants