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.

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

Fix legacy task list position calculation #3491

Merged
merged 8 commits into from
Jul 12, 2023
Merged

Fix legacy task list position calculation #3491

Show file tree
Hide file tree
Changes from 6 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
5 changes: 5 additions & 0 deletions .changeset/smart-points-exercise.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Address scenario in useListInteraction where the position calculation can be incorrect when tasklists appear above legacy task lists
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,})[^`]*$/)
Copy link
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.

Copy link
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.

Copy link
Contributor

@iansan5653 iansan5653 Jul 10, 2023 •
edited
Loading

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 👍

Copy link
Contributor

@iansan5653 iansan5653 Jul 10, 2023 •
edited
Loading

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 regex = new RegExp(`^ {0,3}${fence}${fence[0]}* *$`)
return regex.test(line)
}

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