Fix only the first item gets tasklist-ified issue#1973
Conversation
|
Code looks good to me. I hope i find some time to try it out. |
mejo-
left a comment
mejo-
left a comment
There was a problem hiding this comment.
Apart from my minor coding style comment, looks and works good. Well done 😊
src/nodes/ListItem.js
Outdated
| if (dispatch) { | ||
| dispatch(tr) | ||
| } | ||
| if (dispatch) dispatch(tr) |
There was a problem hiding this comment.
Not sure about our coding style convention here, I thought that we prefer to always use curly brackets for better readability. But I might be wrong :)
There was a problem hiding this comment.
Yes, I'd prefer that, though not enforced through our eslint rules currently.
https://github.com/nextcloud/eslint-config/blob/master/index.js
https://eslint.org/docs/rules/curly
|
This also would fix #903. Just a notice that we don't forget to close the bugeport afterwards. |
src/nodes/ListItem.js
Outdated
| } | ||
|
|
||
| tr.setNodeMarkup(parentList.pos, schema.nodes.list_item, { type: parentList.node.attrs.type === TYPES.CHECKBOX ? TYPES.BULLET : TYPES.CHECKBOX }) | ||
| state.doc.nodesBetween(selection.from, selection.to, (node, pos) => { |
There was a problem hiding this comment.
| state.doc.nodesBetween(selection.from, selection.to, (node, pos) => { | |
| tr.doc.nodesBetween(tr.selection.from, tr.selection.to, (node, pos) => { |
Shouldn't this operate on the transaction instead of the original document?
There was a problem hiding this comment.
With that it would also fix #903 right away it seems, otherwise when you select a block of paragraph nodes and trigger the checklist, they will only get converted to a regular bullet list, as the toggleList call from above is not yet dispatched to the original state.doc
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Works fine, nice.
As mentioned by @juliushaertl, It still feels weird that when selecting multiple lines of raw text, it requires 2 clicks on the "todo list" button to actually get checkboxes.
|
/backport to stable23 |
|
/backport to stable22 |
|
/backport to stable21 |
luka-nextcloud
force-pushed
the
fix/task-list-issue
branch
from
4117659 to
7229fe9
Compare
|
@luka-nextcloud: seems like some file called |
Signed-off-by: Luka Trovic <luka@nextcloud.com>
luka-nextcloud
force-pushed
the
fix/task-list-issue
branch
from
88697df to
fa9603f
Compare
|
The backport to stable22 failed. Please do this backport manually. |
|
The backport to stable21 failed. Please do this backport manually. |
|
TODO: backports to stable21 and stable22 need to be done manually. |
|
/backport stable22 |
|
/backport to stable22 |
|
/backport to stable21 |
|
The backport to stable22 failed. Please do this backport manually. |
|
The backport to stable21 failed. Please do this backport manually. |
5 participants
Signed-off-by: Luka Trovic luka@nextcloud.com
Resolves: ✨ Text app design review #1075
If you mark a whole list and switch it to a task list, only the first item gets tasklist-ifiedTarget version: master
Summary
Fixed the issue: If you mark a whole list and switch it to a task list, only the first item gets tasklist-ified
