-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[WORK IN PROGRESS] - List Improvements #3819
Conversation
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
The current new behavior would be the following: if the cursor is at the start and there is no direct sibling list item on the previous linein this case, the current list item will just be lifted up one level
if the cursor is at the start of a list item and the previous list item is on the same level plus doesn't have a sublistin this case, the list items plus their paragraphs are merged
in case the merged list item had child items those are lifted up accordingly:
|
Just a quick update: This PR is almost ready to be merged. Since we have a strict deadline right now for Reason for that is, that those changes are quite dangerous to get merged fast without testing as a lot of editors could break if some bugs occur. When the |
There seems to be a bug / unintuitive behavior in the preview https://deploy-preview-3819--tiptap-embed.netlify.app/preview/Overview/Installation:
I think the approach for list-items in ReMirror is quite user-friendly:
|
Thanks for pointing this one out @bkempe - will fix that behavior and get those list changes into 2.1.0-rc.0 by the end of the week! |
Any updates on when this is going to land? |
Further findings I thought should be considered regarding a (I think) previously unknown list copy/paste bug: #3128 (comment) |
Courtesy of @AdventureBeard I tried the extensionized version of these changes with Tested with this input
Findings so far:Unresolved:
Resolved:
Introduced problems:
Image for #3 Changes in behavior
ThoughtsI wonder if a ProseMirror plugin to |
Friendly bump. |
Regarding this item on the list:
Is this not a more generic problem? It affects all sorts of lists as well as blockquotes. Is this something that can be fixed in a more generalizable way? Or does the (hypothetical) fix need to be applied to each I noticed the same sort of issue affects |
@bdbch I implemented your changes as an Extension to try to fix the problems related to I'd encourage everyone interested in this issue to try this extension and report back their findings. Try the extension yourself! I'm not sure why, but some of the issues I was previously seeing no longer seem to be appearing. Perhaps I'm testing improperly, or perhaps I previously tested improperly? The only issues I now see are:
cant-delete-parent-items.mp4
interstitial-area-pasting.mp4
|
@slapbox The extension works great! One minor issue I've found is that pressing backspace while the cursor is at the first character of the second line of a paragraph in a list item containing multiple paragraphs deletes the previous list item. IMO a list item containing multiple paragraphs is an invalid state anyway (with this extension the only way I can get get to it is by Ctrl+Shift+V pasting multiple paragraphs into a list item), but it should probably be considered if this state is to be allowed. |
Great find @kalda341. I added a (poorly named) check for that but haven't tested the new version: // avoid unindenting at start of paragraph if paragraph is not the first in the listItem
if (atStartOfSecondOrLaterParagraph(state)) {
return commands.joinBackward();
} This fix will not help users who use the default
|
I discovered there are some additional remaining problems:
PS: I believe that the best approach to releasing these changes is to release them as something like |
@slapbox Yes, I suggest everyone makes the content change - it seems like a much more sensible default. Those changes have improved things enough for me that I'm going to trial them in production. Wish me luck! |
@bdbch I noticed you've done some more work on this; thank you very much for it! I know I've commented quite extensively on many related issues here to the point it would be overwhelming to respond to everything, but I hope you could find time to reply to this comment, which I feel is the summation of dozens of hours of investigation into this subject. Most especially to point 1 and the idea to release these changes as an extension for guaranteed backwards compatibility for all users. |
Hey @slapbox – I've read all your comments on this PR. Good work already. I'll need some time to check out your open issues. I'm heavily packed with a project rn. I released the first batch of this PR with 2.1.0-rc.9 and will continue working on branch |
Thanks for your reply and your hard work @bdbch! Unfortunately, as released these changes break our existing work and lock us into being stuck at I'm not sure right now what would be involved for me to fix our project to work with As much as I want these changes integrated, I really think that the way they're released right now should be rolled back. I'd totally support a release-candidate release of them via an extension though, because that would be guaranteed not to break anything. Otherwise I'd suggest they wait for Edit: I also think these changes fail to account for Edit2: I disabled the There are also some other cases the gist handled which the released code does not like backspacing away a |
@bdbch I really appreciate you coming back to this - it will be great to have an official solution to these issues.
For this reason, I'm going to stick with @slapbox's extension for now instead of upgrading, though I hope these issues can be resolved officially. Given this is a potentially breaking change for some users this late in an RC, I too support including the fixes as an extension. Including the fixes in 2.2 would be much more palatable, especially if they are a bit more complete and robust. Thanks again @bdbch, I really hope we can get to a great solution that works for everyone. |
I found that the problems I thought were interference from But I still feel that this change should be reverted. It may fix some issues but it generally makes lists even more unpredictable to work with since the partial fixes introduce new edge cases users won't have seen before. I still really think it should be rolled back deferred until |
Good point. I already thought about reverting this for now. In the meantime: I created a PR (see #4140) which should make handling positions (specially nested) easier in the future. As soon as I'm done with this, I'll continue work on the lists. In the end I want lists to not be unpredictable anymore and don't break / lock users into unavoidable traps in the content while only navigating with the keyboard. (this would also be included in 2.1.0 / 2.2.0) |
Just made a fix for this in a new branch.
Also made a fix for this in said branch
I tried this with a listItem with 2 paragraphs. On backspace, it seems to work fine - just delete is kind of funky. need to look into this. in general this blocks a stable release right now but I also don't really want to revert anything or move it into an optional package. List behavior on vanilla Prosemirror is really weird and having this as an optional extension would really not fulfill what I'd expect Tiptap to do (I install Tiptap + List extensions and I expect a well working list editing behavior already). I think I'd rather think about how we can share all that list logic between the list and taskList extensions.
Can you give me more information what exactly is breaking right now or even create a new ticket out of this? |
Maybe @rfgamaral can also drop his 5 cents in here since he pointed me towards this issue. I'd like lists to work reliable but also work as expected from the enduser. |
@bdbch The discussion is a bit long already, and I'm not exactly sure what you are asking me. Can you please rephrase? |
I think the main discussion right now is to if the list changes I made should be a separate extension so it doesn't break existing projects or it should be included in the core / list extension itself. The bugs described in the thread here are already handled as far as I can see. |
@bdbch I think releasing it integrated into the existing lists extension would be fine since it turns out my issues were due to my own mistake - but I don't see how it would work with taskLists in that case which was another reason for my suggestion of a separate extension, or otherwise it seems like there's no way to avoid code duplication. I'd not release it as part of the existing lists extension until
Tell me about it! I don't understand how it's considered intended behavior since ProseMirror is otherwise so predictable and polished. |
@bdbch 2.2.0 would be great. |
I think fixes like this belong in the core extensions. It doesn't make sense to me that I need to install multiple extensions to get lists behaviour working as they should by default. What problem does the a separate extension solve? Code duplication for task lists? If that's the only problem, I think we should have code duplication for lists and task lists in interim releases (so that we can get these fixes out to everyone ASAP), and iterate on that, perhaps finding a better solution that doesn't rely on an extra extension? |
I solved this by moving those helper functions handling the list positioning into the core where it belongs to (as it is used by multiple extension and could be used by other list extensions in the future) I think we'll stick to it being in core and bump the version higher up so people know about those breaking changes regarding lists and can react accordingly. |
What kind of breaking changes are we talking about here, @bdbch? Can you give some examples? |
This PR will implement several list item improvements to make handling with lists easier.
Todo