Feature/858/checkboxtoggle #861

newhinton · 2022-05-22T10:22:02Z

fixes #858

Signed-off-by: Felix Nüsse <[email protected]>

korelstar

Thanks for this PR! I really like it! Here are some remarks for improving it:

Changing a checkbox only works once for me. After that, any other change is done visually, but not in the note. I have to reload to proceed.
Please respect the read-only attribute. The checkboxes should not be editable if read-only mode is active.
Could you please change the mouse cursor? Currently it is text selection, but I think it should be the default pointer.
Would it be possible to use the same checkbox design like in edit mode?

package-lock.json

src/components/EditorMarkdownIt.vue

newhinton · 2022-05-30T20:17:36Z

Changing a checkbox only works once for me. After that, any other change is done visually, but not in the note. I have to reload to proceed.

~~That is interesting. I can reproduce, however, the onEdit call does take the correct content, but it seems it does not save it. I have to figure out why.~~ I was wrong, that was the editor not the preview

Would it be possible to use the same checkbox design like in edit mode?

Yes, but no. :D I have already changed the checkbox design in #790 to use rounded corners, so that the editor matches the preview. If you dont object, i would like to do it that way:

Editor:

Preview:

(the editor has a slightly smaller border)

Signed-off-by: Felix Nüsse <[email protected]>

newhinton · 2022-05-31T06:23:29Z

How do i test the read-only attribute? I have passed it just like the editor so it 'should' be fine, but i have never encountered a read only note. Are those shared from other people?

use regex for checking if todo Signed-off-by: Felix Nüsse <[email protected]>

Signed-off-by: Felix Nüsse <[email protected]>

newhinton · 2022-05-31T08:01:14Z

So, i fixed the single click only bug. Sadly i had to introduce a timeout function so that the v-html can render and the new content is actually found with the checkboxes. If you have a better idea please tell me.

Also i found a very strange bug, but i think this is the issue with MarkdownIT.

 - [x] test
 - [x] test2
 [ ] test
 
 
 test

This note will have 2 checkboxes, as expected, but the label of the second one extends to a second line. Can you confirm this behaviour on your end?

Signed-off-by: Felix Nüsse <[email protected]>

src/components/EditorMarkdownIt.vue

korelstar · 2022-07-10T14:49:26Z

src/components/EditorMarkdownIt.vue

+				if (target.checked) {
+					returnValue = returnValue.replace('[ ]', '[x]')
+					returnValue = returnValue.replace('[\u00A0]', '[x]')
+				} else {
+					returnValue = returnValue.replace('[x]', '[ ]')
+					returnValue = returnValue.replace('[X]', '[ ]')
+				}


This does not work correctly if there is a [ ] or [x] in the item's text, e.g. - [x] this is an example [x] with checkbox. In this case, the text is also changed. In order to fix this, it must be assured, that only a single replace is done.

As far as i know the .replace() function only replaces the first occurence, when no regex is used. Even if a regex is applied, when the global-flag is missing it will also only chamge the first occurence. So this should be fine, did your test yield different results?

Please try my example. It doesn't work correctly. But I did not found out why.

It works just fine for me, have you cleared the cache? (I had problems when the developer console wasnt open, it was using some broken hybrid version from cache)

It only has the behaviour you describe if we have both an uppercase and a lowercase X in the line. I will have to fix this particular issue though.

I've found out that it happens on sub-items only, see the following example:

- [ ] check [ ] box (works) - [ ] sub check [ ] box (does not work) - [ ] check [ ] box (works)

check [ ] box (works)

sub check [ ] box (does not work)

check [ ] box (works)

I also cannot reproduce this. Is the sub-element supposed to be indented? Because on my build it behaves identical to the other two (including the checkbox behaviour)

Yes, the second item should be indented. See this screenshot:

Co-authored-by: korelstar <[email protected]>

Signed-off-by: Felix Nüsse <[email protected]>

korelstar · 2022-07-10T19:18:16Z

So, i fixed the single click only bug. Sadly i had to introduce a timeout function so that the v-html can render and the new content is actually found with the checkboxes. If you have a better idea please tell me.

I think it's fine, for now.

Also i found a very strange bug, but i think this is the issue with MarkdownIT.
 - [x] test
 - [x] test2
 [ ] test
 
 
 test
This note will have 2 checkboxes, as expected, but the label of the second one extends to a second line. Can you confirm this behaviour on your end?

Yes, I can reproduce this. But I agree that this is caused by markdown-it.

korelstar

How do i test the read-only attribute? I have passed it just like the editor so it 'should' be fine, but i have never encountered a read only note. Are those shared from other people?

Yes, that's the typical use case. But you could test this also by manually removing write permission to the note's file directly on the file system.

I've added a code change suggestion in order to improve this case: The checkboxes should be disabled if and only if the note is read-only.

src/components/EditorMarkdownIt.vue

Signed-off-by: Felix Nüsse <[email protected]>

korelstar · 2022-08-05T20:38:38Z

@newhinton What's the state of this PR? As far as I can see, there is only one bug left: the issue with the sub-items (see #861 (comment)). Any ideas on how to fix this?

korelstar · 2022-08-05T21:17:29Z

@newhinton What's the state of this PR? As far as I can see, there is only one bug left: the issue with the sub-items (see #861 (comment)). Any ideas on how to fix this?

Okay, I found a fix. It was because clicking on a sub-item triggers an event for the sub-item AND for the parent item. I had to stop event propagation.

newhinton added 4 commits May 22, 2022 11:34

find all checkboxes and add listener

89a8f60

Signed-off-by: Felix Nüsse <[email protected]>

allow viewer to update markdown

eb5a779

Signed-off-by: Felix Nüsse <[email protected]>

fix lint

67808dd

Signed-off-by: Felix Nüsse <[email protected]>

allow uppercase X to be used as checkmark

b4fffa5

Signed-off-by: Felix Nüsse <[email protected]>

korelstar requested changes May 29, 2022

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

src/components/EditorMarkdownIt.vue Outdated Show resolved Hide resolved

newhinton changed the title ~~Feature/858/checkboxtoggle~~ Draft: Feature/858/checkboxtoggle May 29, 2022

newhinton changed the title ~~Draft: Feature/858/checkboxtoggle~~ Feature/858/checkboxtoggle May 29, 2022

newhinton marked this pull request as draft May 29, 2022 20:27

newhinton added 3 commits May 30, 2022 22:22

use pointer as cursor

fc134ed

Signed-off-by: Felix Nüsse <[email protected]>

undo package-lock.json

848598b

Signed-off-by: Felix Nüsse <[email protected]>

pass read only attribute

f6fd354

Signed-off-by: Felix Nüsse <[email protected]>

newhinton added 3 commits May 31, 2022 09:07

refactor changeListener-function

ceb2d29

use regex for checking if todo Signed-off-by: Felix Nüsse <[email protected]>

always update via delayed changeListener

131d1ef

Signed-off-by: Felix Nüsse <[email protected]>

remove unnessesary wrapper code

d365091

Signed-off-by: Felix Nüsse <[email protected]>

fix lint

21dd736

Signed-off-by: Felix Nüsse <[email protected]>

newhinton marked this pull request as ready for review May 31, 2022 08:04

korelstar reviewed Jul 10, 2022

View reviewed changes

src/components/EditorMarkdownIt.vue Outdated Show resolved Hide resolved

korelstar reviewed Jul 10, 2022

View reviewed changes

newhinton and others added 2 commits July 10, 2022 17:54

Update src/components/EditorMarkdownIt.vue

8d5c4d3

Co-authored-by: korelstar <[email protected]>

prevent two occurences of X to be replaced

73824cc

Signed-off-by: Felix Nüsse <[email protected]>

korelstar requested changes Jul 10, 2022

View reviewed changes

src/components/EditorMarkdownIt.vue Outdated Show resolved Hide resolved

newhinton added 2 commits July 10, 2022 21:54

disable checkbox on readonly note as suggested by @korelstar

40f56ce

Signed-off-by: Felix Nüsse <[email protected]>

fix wrong inversion of read only state for checkbox handling

22b59f2

Signed-off-by: Felix Nüsse <[email protected]>

korelstar added 2 commits August 5, 2022 23:08

fix sub-items

a14f337

code style

dd810ae

korelstar approved these changes Aug 5, 2022

View reviewed changes

korelstar merged commit dc3672f into master Aug 5, 2022

korelstar deleted the feature/858/checkboxtoggle branch August 5, 2022 21:19

korelstar added this to the 4.5.0 milestone Aug 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/858/checkboxtoggle #861

Feature/858/checkboxtoggle #861

newhinton commented May 22, 2022 •

edited

Loading

korelstar left a comment

newhinton commented May 30, 2022 •

edited

Loading

newhinton commented May 31, 2022

newhinton commented May 31, 2022 •

edited

Loading

korelstar Jul 10, 2022

newhinton Jul 10, 2022

korelstar Jul 10, 2022

newhinton Jul 10, 2022

newhinton Jul 10, 2022

korelstar Jul 10, 2022

newhinton Jul 10, 2022 •

edited

Loading

korelstar Jul 10, 2022

korelstar commented Jul 10, 2022

korelstar left a comment

korelstar commented Aug 5, 2022

korelstar commented Aug 5, 2022

Feature/858/checkboxtoggle #861

Feature/858/checkboxtoggle #861

Conversation

newhinton commented May 22, 2022 • edited Loading

korelstar left a comment

Choose a reason for hiding this comment

newhinton commented May 30, 2022 • edited Loading

newhinton commented May 31, 2022

newhinton commented May 31, 2022 • edited Loading

korelstar Jul 10, 2022

Choose a reason for hiding this comment

newhinton Jul 10, 2022

Choose a reason for hiding this comment

korelstar Jul 10, 2022

Choose a reason for hiding this comment

newhinton Jul 10, 2022

Choose a reason for hiding this comment

newhinton Jul 10, 2022

Choose a reason for hiding this comment

korelstar Jul 10, 2022

Choose a reason for hiding this comment

newhinton Jul 10, 2022 • edited Loading

Choose a reason for hiding this comment

korelstar Jul 10, 2022

Choose a reason for hiding this comment

korelstar commented Jul 10, 2022

korelstar left a comment

Choose a reason for hiding this comment

korelstar commented Aug 5, 2022

korelstar commented Aug 5, 2022

newhinton commented May 22, 2022 •

edited

Loading

newhinton commented May 30, 2022 •

edited

Loading

newhinton commented May 31, 2022 •

edited

Loading

newhinton Jul 10, 2022 •

edited

Loading