Skip to content

Fix legacy task list position calculation #3491

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.

Sign up for GitHub

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

Merged
siddharthkp merged 8 commits into from
Jul 12, 2023
Merged

Fix legacy task list position calculation #3491

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac211f0
Fix bug with legacy task list position calculation
nicolleromero Jul 6, 2023
b456e32
Add test for hierarchy task list combination
nicolleromero Jul 6, 2023
8e5df65
Make check for code fences more robust per spec
nicolleromero Jul 6, 2023
c2e70df
Update to use regex test
nicolleromero Jul 10, 2023
22cc801
Add changeset
nicolleromero Jul 10, 2023
89809a3
Update changeset
nicolleromero Jul 10, 2023
26b488f
add component name in changelog
siddharthkp Jul 11, 2023
f5e867b
Merge branch 'main' into fix-legacy-task-list-checkboxes
siddharthkp Jul 11, 2023
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
80 changes: 80 additions & 0 deletions src/drafts/MarkdownViewer/MarkdownViewer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,56 @@ text before list
- [x] item 1
- [ ] item 2

text after list`
const hierarchyBeforeTaskListNoItemsChecked = `
text before list

\`\`\`[tasklist]
- [ ] item A
- [ ] item B
\`\`\`

- [ ] item 1
- [ ] item 2

text after list`
const hierarchyBeforeTaskListOneItemChecked = `
text before list

\`\`\`[tasklist]
- [ ] item A
- [ ] item B
\`\`\`

- [x] item 1
- [ ] item 2

text after list`
const hierarchyBeforeTaskListNoItemsCheckedTildes = `
text before list

~~~[tasklist]
- [ ] item A
- [ ] item B
\`\`\`
~~~~~~

- [ ] item 1
- [ ] item 2

text after list`
const hierarchyBeforeTaskListOneItemCheckedTildes = `
text before list

~~~[tasklist]
- [ ] item A
- [ ] item B
\`\`\`
~~~~~~

- [x] item 1
- [ ] item 2

text after list`

it('enables checklists by default', () => {
Expand Down Expand Up @@ -75,6 +125,36 @@ text after list`
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(
<MarkdownViewer
dangerousRenderedHTML={htmlObject}
markdownValue={hierarchyBeforeTaskListNoItemsChecked}
onChange={onChangeMock}
disabled
/>,
)
const items = getAllByRole('checkbox')
fireEvent.change(items[0])
await waitFor(() => expect(onChangeMock).toHaveBeenCalledWith(hierarchyBeforeTaskListOneItemChecked))
})

it('calls `onChange` with the updated Markdown when a task is checked and hierarchy is present with tildes', async () => {
const onChangeMock = jest.fn()
const {getAllByRole} = render(
<MarkdownViewer
dangerousRenderedHTML={htmlObject}
markdownValue={hierarchyBeforeTaskListNoItemsCheckedTildes}
onChange={onChangeMock}
disabled
/>,
)
const items = getAllByRole('checkbox')
fireEvent.change(items[0])
await waitFor(() => expect(onChangeMock).toHaveBeenCalledWith(hierarchyBeforeTaskListOneItemCheckedTildes))
})

it('calls `onChange` with the updated Markdown when a task is unchecked', async () => {
const onChangeMock = jest.fn()
const {getAllByRole} = render(
Expand Down
25 changes: 24 additions & 1 deletion src/drafts/MarkdownViewer/_useListInteraction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,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) => {
const match = line.match(/^ {0,3}(`{3,}|~{3,})[^`]*$/)

@iansan5653 iansan5653 Jul 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const match = line.match(/^ {0,3}(`{3,}|~{3,})[^`]*$/)
const match = line.match(/^ *(`{3,}|~{3,})[^`]*$/)

Looks like the spec says that an opening fence can be indented an indefinite number of spaces, but the closing fence can only be indented up to 3.

@nicolleromero nicolleromero Jul 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The spec states that both an opening and closing fence can be indented no more than three spaces:

A fenced code block begins with a code fence, indented no more than three spaces.

The closing code fence may be indented up to three spaces

I also tested and confirmed this is the case.

@iansan5653 iansan5653 Jul 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Huh, the spec has an example with more than three, but I think I misread it - it looks like that doesn't actually make a code block. So, all good 👍

@iansan5653 iansan5653 Jul 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Side note, maybe we also should filter out lines starting with more than 3 spaces?

Ie, this is not a checklist item, but it's also not in a code block:

- [ ] example

This came from this markdown:

    - [ ] example

Probably something to think about for a different PR though

return match ? match[1] : null
}

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

@iansan5653 iansan5653 Jul 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const match = line.match(new RegExp(`^ {0,3}${fence}${fence[0]}* *$`))
const match = line.match(new RegExp(`^ {0,3}${fence} *$`))

I think the closing fence needs to have exactly the same number of delimiters as the opening fence. For example, this is valid:

```one
````
we are still in block `one` here
```

@nicolleromero nicolleromero Jul 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically the closing fence needs to have "at least as many" delimiters as the opening fence:

The content of the code block consists of all subsequent lines, until a closing code fence of the same type as the code block began with (backticks or tildes), and with at least as many backticks or tildes as the opening code fence.

So in the case you mentioned above, the second line would suffice as a closing fence since it has "at least" three backticks (tested in the issue editor on prod and confirmed).

@iansan5653 iansan5653 Jul 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh wow, you are totally right 👍 I definitely misread that

return match !== null
Comment thread
iansan5653 marked this conversation as resolved.
Outdated
}

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

const toggleTaskListItem = (item: TaskListItem): TaskListItem => ({
Expand Down Expand Up @@ -43,9 +54,21 @@ export const useListInteraction = ({
const onToggleItem = useCallback(
(toggledItemIndex: number) => () => {
const lines = markdownRef.current.split('\n')
let currentCodeFence: string | null = null

for (let lineIndex = 0, taskIndex = 0; lineIndex < lines.length; lineIndex++) {
const parsedLine = parseListItem(lines[lineIndex])
const line = lines[lineIndex]

if (!currentCodeFence) {
currentCodeFence = parseCodeFenceBegin(line)
} else if (isCodeFenceEnd(line, currentCodeFence)) {
currentCodeFence = null
continue
}

if (currentCodeFence) continue

const parsedLine = parseListItem(line)

if (!isTaskListItem(parsedLine)) continue

Expand Down