fix nested ordered lists with more than 10 items #104
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akabiru
approved these changes
Jul 8, 2025
| content = content | ||
| .replace(/^\n+/, '') // remove leading newlines | ||
| .replace(/\n+$/, '\n') // replace trailing newlines with just a single one | ||
| .replace(/\n/gm, '\n ') // indent with five spaces (five is important) |
There was a problem hiding this comment.
I'm not sure I follow why "5" spaces is important. Should we expound on that a bit more in the comment or fn description?
There was a problem hiding this comment.
Yeah, 5 spaces is a quick fix. And kinda brute forcing things
1. list item
2. list item 2
- sub item # this works, because the `-` is indented with four spaces, two more than the `2.`
...
10. list item 10
- sub item # this does not work, because the `-` is indented four spaces, only a single space over the `10.`. So we need 5 here instead`So, forcing 5 spaces kinda fixes lists until 100 😬
But I probably can make an algorithm that will handle more gracefully the indentation width.
There was a problem hiding this comment.
Nice one, much more clear. And really nice one on the unit tests! 🥇
brunopagno
force-pushed
the
fix/64414-allow-long-ordered-lists-with-nested-items
branch
from
09554b6 to
4c6e5ba
Compare
brunopagno
force-pushed
the
fix/64414-allow-long-ordered-lists-with-nested-items
branch
from
4c6e5ba to
828b25b
Compare
brunopagno
deleted the
fix/64414-allow-long-ordered-lists-with-nested-items
branch
akabiru
reviewed
Jul 8, 2025
| /** | ||
| * This rule is used to convert ordered list items to markdown. | ||
| * | ||
| * This is based on he turndownService rule for listItems, but modified to |
There was a problem hiding this comment.
Suggested change
| * This is based on he turndownService rule for listItems, but modified to | |
| * This is based on the turndownService rule for listItems, but modified to |
| content = content | ||
| .replace(/^\n+/, '') // remove leading newlines | ||
| .replace(/\n+$/, '\n') // replace trailing newlines with just a single one | ||
| .replace(/\n/gm, '\n ') // indent with five spaces (five is important) |
There was a problem hiding this comment.
Nice one, much more clear. And really nice one on the unit tests! 🥇
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
https://community.openproject.org/projects/communicator-stream/work_packages/64414/activity
What are you trying to accomplish?
Fix an issue where nested ordered lists with more than 10 items would break when persisted.
Screenshots
What approach did you choose and why?
Based on turndown.js implementation of conversion of list items, make the indentation 5 spaces instead of 4. This allows the commonmarker gem used on OpenProject to properly parse large nested ordered lists.
Merge checklist