Skip to content
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

Desktop, Mobile: Fixes #11845: Fixes issues with converting nested lists between bullet and numbered. #11857

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

rabbabansh
Copy link

@rabbabansh rabbabansh commented Feb 18, 2025

Fixes #11845 now, this"

  • test
    • test
    • test
  • test

converts to:

Image

Checklists also convert nested lists fully, rather than just converting the first indent to checklists.

Copy link
Contributor

github-actions bot commented Feb 18, 2025

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@rabbabansh
Copy link
Author

I have read the CLA Document and I hereby sign the CLA.

@rabbabansh rabbabansh force-pushed the fix_nested_list_conversion branch from ed22130 to 357cb63 Compare February 18, 2025 20:49
github-actions bot added a commit that referenced this pull request Feb 18, 2025
@personalizedrefrigerator
Copy link
Collaborator

Thank you for working on this! Be sure to also update the automated tests in packages/editor/ with the expected output.

@rabbabansh
Copy link
Author

Done! Please review

@rabbabansh rabbabansh force-pushed the fix_nested_list_conversion branch from b2505e2 to a9f1d46 Compare February 27, 2025 00:26
@personalizedrefrigerator
Copy link
Collaborator

Thank you! Be sure to update the existing tests, too — one is failing:

➤ YN0000: [@joplin/editor]:   FAIL CodeMirror/markdown/markdownCommands.toggleList.test.ts
➤ YN0000: [@joplin/editor]:   ● markdownCommands.toggleList › should toggle a numbered list without changing its sublists
➤ YN0000: [@joplin/editor]: 
➤ YN0000: [@joplin/editor]:     expect(received).toBe(expected) // Object.is equality
➤ YN0000: [@joplin/editor]: 
➤ YN0000: [@joplin/editor]:     - Expected  - 3
➤ YN0000: [@joplin/editor]:     + Received  + 3
➤ YN0000: [@joplin/editor]: 
➤ YN0000: [@joplin/editor]:       - [ ] Foo
➤ YN0000: [@joplin/editor]:       - [ ] Bar
➤ YN0000: [@joplin/editor]:       - [ ] Baz
➤ YN0000: [@joplin/editor]:     - 	- Test
➤ YN0000: [@joplin/editor]:     + 	- [ ] Test
➤ YN0000: [@joplin/editor]:     - 	- of
➤ YN0000: [@joplin/editor]:     + 	- [ ] of
➤ YN0000: [@joplin/editor]:     - 	- sublists
➤ YN0000: [@joplin/editor]:     + 	- [ ] sublists
➤ YN0000: [@joplin/editor]:       - [ ] Foo
➤ YN0000: [@joplin/editor]: 
➤ YN0000: [@joplin/editor]:       191 |
➤ YN0000: [@joplin/editor]:       192 | 		toggleList(ListType.CheckList)(editor);
➤ YN0000: [@joplin/editor]:     > 193 | 		expect(editor.state.doc.toString()).toBe(
➤ YN0000: [@joplin/editor]:           | 		                                    ^
➤ YN0000: [@joplin/editor]:       194 | 			'- [ ] Foo\n- [ ] Bar\n- [ ] Baz\n\t- Test\n\t- of\n\t- sublists\n- [ ] Foo',
➤ YN0000: [@joplin/editor]:       195 | 		);
➤ YN0000: [@joplin/editor]:       196 | 	});
➤ YN0000: [@joplin/editor]: 
➤ YN0000: [@joplin/editor]:       at Object.<anonymous> (CodeMirror/markdown/markdownCommands.toggleList.test.ts:193:39)

@rabbabansh
Copy link
Author

That tests passes now! I rewrote the expected results for a nested checklist that gives checklists to the sublists as well.

@laurent22
Copy link
Owner

@personalizedrefrigerator, please let me know when you think it's ready to merge, as I don't know enough about the CM6 implementation to review

@rabbabansh
Copy link
Author

any updates @personalizedrefrigerator ?

@personalizedrefrigerator
Copy link
Collaborator

any updates @personalizedrefrigerator ?

The code looks good to me! I plan to test the change locally before approving the pull request.

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Mar 3, 2025

I've tested this locally. I've noticed that changing the list type with the cursor anywhere in the toplevel list (with an empty selection) changes the list types of all sublists. The description of #11845 includes only the case where all text is selected.

  • Note: This is different from how Joplin's Rich Text Editor behaves and thus may not be desired behavior (but to be confirmed by @laurent22). For comparison, moving the cursor to a bulleted list in the Rich Text Editor with bulleted sublists, then clicking "numbered list" only changes the outer list.
  • For example, with this pull request clicking "numbered list" with the following
    - This is a test. Cursor here ->> |
       - Sublist
          - Sub sublist.
    - Testing
    - Test.
    
    produces
    1. This is a test. Cursor here ->> |
       1. Sublist
          1. Sub sublist.
    2. Testing
    3. Test.
    
    Whereas in the Rich Text Editor, it would preserve the bulleted sublists.

Thank you @rabbabansh for the work you've done on this so far!

@laurent22
Copy link
Owner

Not sure what's best here? What do other editors do in this situation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot change nested lists to a different type in new Markdown editor
3 participants