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] Non-standard formatted tasklist items are not identified as tasklist items #3972

Merged
merged 7 commits into from
Nov 28, 2023

Conversation

iulia-b
Copy link
Contributor

@iulia-b iulia-b commented Nov 24, 2023

The MD editor is not considering a valid tasklist item a line considering of an item that contains multiple spaces between the - delimiter and the [ ] textbox. This used to work as expected in the Rails experience - because of the differences in implementation, the [ ] there had a data attribute decorated with the position in the list and was not parsing again the text line, when the textbox was checked.

This PR is changing the regex that parses the line and takes into account 1 or 2 spaces that can be present there. i used {1, 2} instead of * for the repetitions so that we don't consider to be correct tasklist items that are too deformed.

Changelog

New

Changed

Regex for identifying a task in a markdown list in order to accept multiple spaces between the delimiter and the box

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan

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.

Copy link

changeset-bot bot commented Nov 24, 2023

🦋 Changeset detected

Latest commit: 1aad0c1

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

Copy link
Contributor

github-actions bot commented Nov 24, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 104.54 KB (0%)
dist/browser.umd.js 105.09 KB (0%)

@iulia-b iulia-b marked this pull request as ready for review November 24, 2023 14:40
@iulia-b iulia-b requested review from a team and broccolinisoup November 24, 2023 14:40
@github-actions github-actions bot temporarily deployed to storybook-preview-3972 November 24, 2023 14:43 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3972 November 24, 2023 14:43 Inactive
Copy link
Contributor

@iansan5653 iansan5653 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just some small questions that I don't think would be significant/noticeable issues.

@@ -45,7 +45,7 @@ const isNumericListItem = (item: ListItem | null): item is NumericListItem => ty
export const parseListItem = (line: string): ListItem | null => {
const result = listItemRegex.exec(line)
if (!result) return null
const [, leadingWhitespace = '', fullDelimeter, itemNumberStr = '', taskBox = null, text] = result
const [, leadingWhitespace = '', fullDelimeter, itemNumberStr = '', , taskBox = null, text] = result
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are skipping the capture group for the one or two spaces, meaning that the updated Markdown will always have only one space. Do we maybe want to preserve the number of spaces like we do with leadingWhitespace 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.

Addressed in ad9c47f

@@ -29,7 +29,7 @@ const calculateNextListItemStarter = ({leadingWhitespace = '', delimeter, taskBo
* 3. Task box (optional)
* 4. Everything following
*/
export const listItemRegex = /^(\s*)([*-]|(\d+)\.)\s(?:(\[[\sx]\])\s)?(.*)/i
export const listItemRegex = /^(\s*)([*-]|(\d+)\.)(\s{1,2})(?:(\[[\sx]\])\s)?(.*)/i
Copy link
Contributor

@iansan5653 iansan5653 Nov 24, 2023

Choose a reason for hiding this comment

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

Are tab characters valid here? Maybe more correct would be space only ( ). Not sure though. It's a minor thing though since that bug would've already been present before this change.

Suggested change
export const listItemRegex = /^(\s*)([*-]|(\d+)\.)(\s{1,2})(?:(\[[\sx]\])\s)?(.*)/i
export const listItemRegex = /^(\s*)([*-]|(\d+)\.)( {1,2})(?:(\[[\sx]\])\s)?(.*)/i

Copy link
Contributor Author

Choose a reason for hiding this comment

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

\s is identifying tabs as well, i just checked

@joshblack joshblack deleted the iunia/misspaced-checkboxes branch November 28, 2023 19:00
@primer primer bot mentioned this pull request Nov 28, 2023
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.

3 participants