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

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