Skip to content

Conversation

@luka-nextcloud
Copy link
Contributor

Signed-off-by: Luka Trovic luka@nextcloud.com

Summary

Fixed the issue: If you mark a whole list and switch it to a task list, only the first item gets tasklist-ified
image

@azul
Copy link
Contributor

azul commented Dec 1, 2021

Code looks good to me. I hope i find some time to try it out.

Copy link
Member

@mejo- mejo- left a 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 😊

if (dispatch) {
dispatch(tr)
}
if (dispatch) dispatch(tr)
Copy link
Member

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 :)

Copy link
Member

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

@mejo-
Copy link
Member

mejo- commented Dec 1, 2021

This also would fix #903. Just a notice that we don't forget to close the bugeport afterwards.

}

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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Member

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

Copy link
Member

@julien-nc julien-nc left a 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.

@juliusknorr
Copy link
Member

/backport to stable23

@juliusknorr
Copy link
Member

/backport to stable22

@juliusknorr
Copy link
Member

/backport to stable21

@mejo-
Copy link
Member

mejo- commented Dec 8, 2021

@luka-nextcloud: seems like some file called patch) { somehow sneaked itself into your commit 😉

Signed-off-by: Luka Trovic <luka@nextcloud.com>
Copy link
Member

@mejo- mejo- left a 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 👌

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@mejo-
Copy link
Member

mejo- commented Dec 9, 2021

TODO: backports to stable21 and stable22 need to be done manually.

@mejo-
Copy link
Member

mejo- commented Dec 16, 2021

/backport stable22

@mejo-
Copy link
Member

mejo- commented Dec 16, 2021

/backport to stable22

@mejo-
Copy link
Member

mejo- commented Dec 16, 2021

/backport to stable21

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

✨ Text app design review

6 participants