-
Notifications
You must be signed in to change notification settings - Fork 108
Fix only the first item gets tasklist-ified issue #1973
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
Conversation
|
Code looks good to me. I hope i find some time to try it out. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
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 |
4117659 to
7229fe9
Compare
|
@luka-nextcloud: seems like some file called |
Signed-off-by: Luka Trovic <luka@nextcloud.com>
88697df to
fa9603f
Compare
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.
Works like a charm 👌
|
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. |
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
