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.

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

Conversation

nicolleromero
Copy link
Contributor

@nicolleromero nicolleromero commented Jul 6, 2023

This PR fixes a bug where the combination of a tasklist + legacy task list in the side panel would cause a miscalculation of the checkbox position and result in the wrong checkbox being updated. Preventing checkboxes within code blocks from being included in the position calculation prevents this bug and other potential bugs (e.g., where a user includes the - [ ] checkbox notation within a code block).

Note: this fix was tested and works in both the previous and new issue viewer in Memex.

Screenshots

Before

Checkbox.bug.before.fix.mov

After

Checkbox.bug.after.fix.mov

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@nicolleromero nicolleromero requested review from a team and mperrotti July 6, 2023 15:48
@changeset-bot
Copy link

changeset-bot bot commented Jul 6, 2023

🦋 Changeset detected

Latest commit: f5e867b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 102.31 KB (0%)
dist/browser.umd.js 102.88 KB (0%)

@nicolleromero nicolleromero temporarily deployed to github-pages July 6, 2023 15:54 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 6, 2023 15:55 Inactive
Copy link
Contributor

@JelloBagel JelloBagel left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this bug! ✨

Looks good to me. Should we add a test for this?

@@ -4,6 +4,8 @@ import {ListItem, listItemToString, parseListItem} from '../MarkdownEditor/_useL

type TaskListItem = ListItem & {taskBox: '[ ]' | '[x]'}

const isCodeBlockDelimiter = (line: string) => line.trimStart().startsWith('```')
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec is a little looser than this, so there will be some types of code blocks that don't get caught.

They can be delimited by tildes instead of backticks:

~~~
example
~~~

They can also have more than three backticks:

````
example
````

In this case, you'd need to keep track of how long the starting delimiter is so that you can ensure the ending delimiter is the same length.

Copy link
Contributor Author

@nicolleromero nicolleromero Jul 6, 2023

Choose a reason for hiding this comment

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

Great point! Will make that update.

@nicolleromero
Copy link
Contributor Author

nicolleromero commented Jul 6, 2023

Thanks for fixing this bug! ✨

Looks good to me. Should we add a test for this?

@JelloBagel Tests added. :)

@nicolleromero nicolleromero temporarily deployed to github-pages July 6, 2023 16:52 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 6, 2023 16:52 Inactive
@nicolleromero nicolleromero force-pushed the fix-legacy-task-list-checkboxes branch from d2a6ce0 to a99fb3e Compare July 6, 2023 17:34
@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 6, 2023 17:38 Inactive
@nicolleromero nicolleromero force-pushed the fix-legacy-task-list-checkboxes branch from a99fb3e to 8e5df65 Compare July 6, 2023 17:40
@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 6, 2023 17:41 Inactive
@nicolleromero nicolleromero temporarily deployed to github-pages July 6, 2023 17:45 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 6, 2023 17:46 Inactive
@nicolleromero nicolleromero requested a review from iansan5653 July 6, 2023 20:57
@joshblack joshblack requested a review from siddharthkp July 7, 2023 20:22
@joshblack
Copy link
Member

FYI @siddharthkp for release next week this would be great to get in!

@@ -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

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

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

}

const isCodeFenceEnd = (line: string, fence: string) => {
const match = line.match(new RegExp(`^ {0,3}${fence}${fence[0]}* *$`))
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(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
```

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

Copy link
Contributor

@iansan5653 iansan5653 Jul 10, 2023

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

src/drafts/MarkdownViewer/_useListInteraction.ts Outdated Show resolved Hide resolved
@nicolleromero nicolleromero temporarily deployed to github-pages July 10, 2023 17:37 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 10, 2023 17:38 Inactive
@nicolleromero
Copy link
Contributor Author

FYI @siddharthkp for release next week this would be great to get in!

Thanks, @joshblack! I added a changeset, but validation appears to be failing. Anything I should update?

@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 10, 2023 17:46 Inactive
@nicolleromero nicolleromero temporarily deployed to github-pages July 10, 2023 17:49 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 10, 2023 17:50 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages July 11, 2023 09:04 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3491 July 11, 2023 09:05 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants