Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/funny-fishes-appear.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

MarkdownEditor & MarkdownViewer: Update new line identifier for formatting markdown
98 changes: 98 additions & 0 deletions src/__tests__/hooks/_useListEditing.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import {parseListItem, listItemToString, ListItem} from '../../drafts/MarkdownEditor/_useListEditing'

describe('parseListItem', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

it('should return null for a line that is not a list item', () => {
expect(parseListItem('This is a test line')).toBeNull()
})

it('should parse a line that is a numbered list item', () => {
expect(parseListItem('1. This is a test line')).toEqual({
leadingWhitespace: '',
text: 'This is a test line',
delimeter: 1,
middleWhitespace: ' ',
taskBox: null,
})
})

it('should parse a line that is a numbered list item and multiple spaces within', () => {
expect(parseListItem('1. This is a test line')).toEqual({
leadingWhitespace: '',
text: 'This is a test line',
delimeter: 1,
middleWhitespace: ' ',
taskBox: null,
})
})

it('should parse a line that is a bulleted list item', () => {
expect(parseListItem('* This is a test line')).toEqual({
leadingWhitespace: '',
text: 'This is a test line',
delimeter: '*',
middleWhitespace: ' ',
taskBox: null,
})
})

it('should parse a line that is a task list item', () => {
expect(parseListItem('- [x] This is a test line')).toEqual({
leadingWhitespace: '',
text: 'This is a test line',
delimeter: '-',
middleWhitespace: ' ',
taskBox: '[x]',
})

// Up to 4 spaces are supported
expect(parseListItem('- [x] This is a test line')).toEqual({
leadingWhitespace: '',
text: 'This is a test line',
delimeter: '-',
middleWhitespace: ' ',
taskBox: '[x]',
})

// 5 spaces are not supported
expect(parseListItem('- [x] This is a test line')).toEqual({
leadingWhitespace: '',
text: ' [x] This is a test line',
delimeter: '-',
middleWhitespace: ' ',
taskBox: null,
})

// Tabs are supported
expect(parseListItem('- [x] This is a test line')).toEqual({
leadingWhitespace: '',
text: 'This is a test line',
delimeter: '-',
middleWhitespace: ' ',
taskBox: '[x]',
})
})
})

describe('listItemToString', () => {
it('should convert a list item to a string', () => {
const item = {
leadingWhitespace: '',
text: 'This is a test line',
delimeter: 1,
middleWhitespace: ' ',
taskBox: null,
}
expect(listItemToString(item)).toBe('1. This is a test line')
})

it('should convert a task list item to a string', () => {
const item = {
leadingWhitespace: '',
text: 'This is a test line',
delimeter: '-',
middleWhitespace: ' ',
taskBox: '[x]',
} as ListItem
expect(listItemToString(item)).toBe('- [x] This is a test line')
})
})
43 changes: 43 additions & 0 deletions src/__tests__/hooks/_useListInteraction.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import {parseCodeFenceBegin, isCodeFenceEnd} from '../../drafts/MarkdownViewer/_useListInteraction'

describe('parseCodeFenceBegin', () => {
it('should return null for a line without a code fence', () => {
expect(parseCodeFenceBegin('This is a test line without a code fence')).toBeNull()
})

it('should return the code fence for a line with a code fence', () => {
expect(parseCodeFenceBegin('```This is a test line with a code fence')).toBe('```')
})

it('should return the code fence for a line with a code fence and leading spaces', () => {
expect(parseCodeFenceBegin(' ~~~This is a test line with a code fence and leading spaces')).toBe('~~~')
})

it('should return null for a line with more than 3 leading spaces before the code fence', () => {
expect(
parseCodeFenceBegin(' ```This is a test line with more than 3 leading spaces before the code fence'),
).toBeNull()
})
})

describe('isCodeFenceEnd', () => {
it('should return true for a line that ends a code fence', () => {
expect(isCodeFenceEnd('```', '```')).toBe(true)
})

it('should return false for a line that does not end a code fence', () => {
expect(isCodeFenceEnd('This is a test line', '```')).toBe(false)
})

it('should return true for a line that ends a code fence with leading spaces', () => {
expect(isCodeFenceEnd(' ~~~', '~~~')).toBe(true)
})

it('should return true for a line that ends a code fence with different new line characteres', () => {
expect(isCodeFenceEnd('~~~', '~~~')).toBe(true)
})

it('should return false for a line with more than 3 leading spaces before the code fence end', () => {
expect(isCodeFenceEnd(' ```', '```')).toBe(false)
})
})
2 changes: 1 addition & 1 deletion src/drafts/MarkdownEditor/_useIndenting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const useIndenting = ({emitChange}: UseIndentingSettings): UseIndentingRe
const [start, end] = getSelectedLineRange(textarea)
const updatedLines = textarea.value
.slice(start, end)
.split('\n')
.split(/\r?\n/)
.map(line => (event.shiftKey ? dedent(line) : indent(line)))
.join('\n')

Expand Down
8 changes: 4 additions & 4 deletions src/drafts/MarkdownEditor/_useListEditing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ export const parseListItem = (line: string): ListItem | null => {
}

export const listItemToString = (item: ListItem) =>
`${item.leadingWhitespace}${typeof item.delimeter === 'number' ? `${item.delimeter}.` : item.delimeter}${
item.middleWhitespace
}${item.taskBox || ''} ${item.text}`
typeof item.delimeter === 'number'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@iansan5653 iansan5653 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the intentional "fixes" and added tests here and here

? `${item.leadingWhitespace}${`${item.delimeter}.`}${item.middleWhitespace}${item.text}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] extra string nesting is unnecessary here

Suggested change
? `${item.leadingWhitespace}${`${item.delimeter}.`}${item.middleWhitespace}${item.text}`
? `${item.leadingWhitespace}${item.delimeter}.${item.middleWhitespace}${item.text}`

: `${item.leadingWhitespace}${item.delimeter}${item.middleWhitespace}${item.taskBox || ''} ${item.text}`

/**
* Provides support for list editing in the Markdown editor. This includes inserting new
Expand All @@ -79,7 +79,7 @@ export const useListEditing = ({emitChange}: UseListEditingSettings): UseListEdi

// Strip off the leading newline by adding 1
const followingText = textarea.value.slice(currentLineEnd + 1)
const followingLines = followingText.split('\n')
const followingLines = followingText.split(/\r?\n/)

const followingNumericListItems: Array<NumericListItem> = []
let prevItemNumber = currentLineItem.delimeter
Expand Down
16 changes: 16 additions & 0 deletions src/drafts/MarkdownViewer/MarkdownViewer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ text before list
- [ ] item 2

text after list`
const noItemsCheckedWithSpecialNewLineMarkdown = `\ntext before list\r\n\r\n- [ ] item 1\n- [ ] item 2\r\n\r\ntext after list`
const hierarchyBeforeTaskListNoItemsChecked = `
text before list

Expand Down Expand Up @@ -125,6 +126,21 @@ text after list`
await waitFor(() => expect(onChangeMock).toHaveBeenCalledWith(firstItemCheckedMarkdown))
})

it('calls `onChange` with the updated Markdown when a task is checked in a text with different new line chars', async () => {
const onChangeMock = jest.fn()
const {getAllByRole} = render(
<MarkdownViewer
dangerousRenderedHTML={htmlObject}
markdownValue={noItemsCheckedWithSpecialNewLineMarkdown}
onChange={onChangeMock}
disabled
/>,
)
const items = getAllByRole('checkbox')
fireEvent.change(items[0])
await waitFor(() => expect(onChangeMock).toHaveBeenCalledWith(firstItemCheckedMarkdown))
})

it('calls `onChange` with the updated Markdown when a task is checked and hierarchy is present', async () => {
const onChangeMock = jest.fn()
const {getAllByRole} = render(
Expand Down
8 changes: 4 additions & 4 deletions src/drafts/MarkdownViewer/_useListInteraction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ import {ListItem, listItemToString, parseListItem} from '../MarkdownEditor/_useL
type TaskListItem = ListItem & {taskBox: '[ ]' | '[x]'}

// Make check for code fences more robust per spec: https://github.github.com/gfm/#fenced-code-blocks
const parseCodeFenceBegin = (line: string) => {
export const parseCodeFenceBegin = (line: string) => {
const match = line.match(/^ {0,3}(`{3,}|~{3,})[^`]*$/)
return match ? match[1] : null
}

const isCodeFenceEnd = (line: string, fence: string) => {
export const isCodeFenceEnd = (line: string, fence: string) => {
const regex = new RegExp(`^ {0,3}${fence}${fence[0]}* *$`)
return regex.test(line)
}

const isTaskListItem = (item: ListItem | null): item is TaskListItem => typeof item?.taskBox === 'string'
export const isTaskListItem = (item: ListItem | null): item is TaskListItem => typeof item?.taskBox === 'string'

const toggleTaskListItem = (item: TaskListItem): TaskListItem => ({
...item,
Expand Down Expand Up @@ -53,7 +53,7 @@ export const useListInteraction = ({

const onToggleItem = useCallback(
(toggledItemIndex: number) => () => {
const lines = markdownRef.current.split('\n')
const lines = markdownRef.current.split(/\r?\n/)
let currentCodeFence: string | null = null

for (let lineIndex = 0, taskIndex = 0; lineIndex < lines.length; lineIndex++) {
Expand Down