Update new line identifier in MarkdownViewer and MarkdownEditor #3987
Conversation
🦋 Changeset detectedLatest commit: 9d07bf4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
src/InlineAutocomplete/utils.ts
Outdated
|
|
||
| const singleWordTriggerTerminators = new Set([' ', '\n']) | ||
| const multiWordTriggerTerminators = new Set(['.', '\n']) | ||
| const singleWordTriggerTerminators = new Set([' ', '\n', '\r\n']) |
There was a problem hiding this comment.
not sure these are needed but it looks safe to me + no tests are failing
There was a problem hiding this comment.
It technically shouldn't be necessary because I think this issue only shows up with templates when they are written offline in external editors. That said, it doesn't hurt to add it.
size-limit report 📦
|
| `${item.leadingWhitespace}${typeof item.delimeter === 'number' ? `${item.delimeter}.` : item.delimeter}${ | ||
| item.middleWhitespace | ||
| }${item.taskBox || ''} ${item.text}` | ||
| typeof item.delimeter === 'number' |
There was a problem hiding this comment.
I've split this on 2 lines for readability + different handling : in the case of numbered list, i chose to ignore the "middle" spacing and format the MD correctly - because it is displayed in the correct way, ignoring the extra spaces
There was a problem hiding this comment.
Not sure I totally understand this. Aren't those extra spaces always rendered as a single space? I figure we'd want to keep the user's raw markdown as close as possible to what they actually wrote when we modify it.
There was a problem hiding this comment.
Yes, this changes the number of spaces for lists that are numbered. I intentionally did this - not sure it's correct but it was a subjective thinking that lists with numbers , given they're also displayed with single space, might as well be "fixed"
| @@ -0,0 +1,88 @@ | |||
| import {parseListItem, listItemToString, ListItem} from '../../drafts/MarkdownEditor/_useListEditing' | |||
|
|
|||
| describe('parseListItem', () => { | |||
| item.middleWhitespace | ||
| }${item.taskBox || ''} ${item.text}` | ||
| typeof item.delimeter === 'number' | ||
| ? `${item.leadingWhitespace}${`${item.delimeter}.`} ${item.text}` |
There was a problem hiding this comment.
I think the delimiter is the dot, so this would add an extra dot:
| ? `${item.leadingWhitespace}${`${item.delimeter}.`} ${item.text}` | |
| ? `${item.leadingWhitespace}${item.delimeter} ${item.text}` |
There was a problem hiding this comment.
The delimiter is the number. The dot was there before as well. There is a test covering this
There was a problem hiding this comment.
Oh yeah, definitely see that now. That's what I get for reviewing from mobile 😄
| item.middleWhitespace | ||
| }${item.taskBox || ''} ${item.text}` | ||
| typeof item.delimeter === 'number' | ||
| ? `${item.leadingWhitespace}${`${item.delimeter}.`}${item.middleWhitespace}${item.text}` |
There was a problem hiding this comment.
[nit] extra string nesting is unnecessary here
| ? `${item.leadingWhitespace}${`${item.delimeter}.`}${item.middleWhitespace}${item.text}` | |
| ? `${item.leadingWhitespace}${item.delimeter}.${item.middleWhitespace}${item.text}` |
siddharthkp
left a comment
siddharthkp
left a comment
There was a problem hiding this comment.
The changes are scoped to drafts/MarkdownEditor and drafts/MarkdownEditor, I trust @iansan5653's review on them :)
Hubbers reported that the new Issues UI was sometimes not responding to checking boxes. After investigating this with @japf we realized that it was due to the fact that those templates were written on a Windows machine and contained
\r\nas a newline character, which was not parsed correctly inside MDViewerChangelog
New
Changed
\nto/\r?\n/throughout markdown handling related filesRemoved
Rollout strategy
Testing & Reviewing
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.