Desktop: Resolves #4813 : skip empty lines while converting selection to list #4832
Conversation
| @@ -89,6 +89,7 @@ export default function useCursorUtils(CodeMirror: any) { | |||
|
|
|||
| for (let j = 0; j < lines.length; j++) { | |||
| const line = lines[j]; | |||
| if (!line) continue; | |||
There was a problem hiding this comment.
Maybe it should be break instead?
What'd happen if you select the text below and convert it to a list?
list
list
not list
There was a problem hiding this comment.
What'd happen if you select the text below and convert it to a list?
After the fix:
Case 1: Only 1st and 2nd lines are selected
Both 1st and 2nd lines will be converted to list;
Case 2: All four lines are selected
"All except third (empty)" line will be converted to list.
Before fix : in case 2 above; all 4 lines will be converted to list
There was a problem hiding this comment.
Maybe it should be
breakinstead?
I'm not sure; It depends on what is the expected behaviour for the case you described?
should I change it to break ?
There was a problem hiding this comment.
I just checked the behaviour of google docs and microsoft word. It looks like Word behaves with the continue behaviour. Google docs just adds a bullet to every line.
I have no personal preference, but someone else might. It could probably be worth putting this to a poll on the forum.
There was a problem hiding this comment.
LInk to the poll on joplin forum.
adarsh-sgh
changed the title
|
By the way, from the look of it there's again an opportunity to add tests here. As I understand a good part of the function simply processes some strings so that could be extracted to a function and properly unit tested. Maybe with a bit of extra effort it's even possible to setup tests for the whole hook. |
|
I've added the tests and some modifications are done in the file to be tested to implement tests. I manually tested inserting and removing lists to ensure that i did not broke anything while trying to test it. one additional test will be added when result of this poll is finalised. |
|
For the list behaviour, it seems the most common across editors is this one:
So could you implement it that way please? |
|
I have made changes to get the behaviour as asked by laurent sir in the above message. |
| @@ -1,6 +1,24 @@ | |||
| import markdownUtils from '@joplin/lib/markdownUtils'; | |||
| import Setting from '@joplin/lib/models/Setting'; | |||
|
|
|||
| export function modifyListLines(lines: string[],num: number,string1: string) { | |||
There was a problem hiding this comment.
Please could you give a descriptive name to "string1"?
| export function modifyListLines(lines: string[],num: number,string1: string) { | ||
| for (let j = 0; j < lines.length; j++) { | ||
| const line = lines[j]; | ||
| if (!line && j == lines.length - 1) continue; |
| @@ -1,20 +1,21 @@ | |||
| import markdownUtils from '@joplin/lib/markdownUtils'; | |||
| import Setting from '@joplin/lib/models/Setting'; | |||
| export function modifyListLines(lines: string[],num: number,string1: string) { | |||
| export function modifyListLines(lines: string[],num: number,listSymbol: string) { | |||
| console.log(listSymbol,':string'); | |||
Before
After Fix
Steps:
Copy paste following content to joplin
item1
item2
item3
a paragraph
Take cursor to beggining of document and press
shift+down arrowthrice to select 1st 3 lines.Toggle list using editor toolbar.