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

Remove extra soft break for tasklist #8142

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Remove extra soft break for tasklist #8142

merged 1 commit into from
Jun 23, 2022

Conversation

black-desk
Copy link
Contributor

Browser will display the extra newline character between checkbox and
text as a space, which make tasklist items cannot be aligned.

I just remove it.

图片

before:
图片
after:
图片

@jgm
Copy link
Owner

jgm commented Jun 23, 2022

This seems a good change! We need tests to be updated before CI will pass.

Browser will display the extra newline character between checkbox and
text as a space, which make tasklist items cannot be aligned.

I just remove it.
@jgm
Copy link
Owner

jgm commented Jun 23, 2022

Thanks!

@mb21
Copy link
Collaborator

mb21 commented Jun 24, 2022

But usually you would want space between the check-mark and the text?

Also, pandoc markdown was according to GitHub Markdown spec before this PR: https://github.github.com/gfm/#task-list-items-extension-

@black-desk
Copy link
Contributor Author

black-desk commented Jun 24, 2022

I think the space between text and box should be done by css but not literal space, just like normal list item:

图片

@tarleb
Copy link
Collaborator

tarleb commented Jun 24, 2022

I think the space between text and box should be done by css but not literal space.

Agreed, but this brings us into a dilemma: either we break compatibility with the GFM spec that gave rise to this feature, or we produce overly-specific HTML.

My preference is with keeping this PR, but the choice should be an informed decision.

@black-desk
Copy link
Contributor Author

My preference is with keeping this PR, but the choice should be an informed decision.

Agreed, but I not familiar with Haskell at all. I not even understand the line I changed :(

Sorry, I cannot help :(

@tarleb
Copy link
Collaborator

tarleb commented Jun 24, 2022

No worries. What I mean is just that it's important to discuss the issue that @mb21 brought up. From my point of view, there are no further changes necessary.

@jgm
Copy link
Owner

jgm commented Jun 24, 2022

Thanks for bringing this up, I hadn't considered this possible drawback.
I think it's okay if our HTML output departs slightly from GitHub's as long as semantically it's the same. However, we do want to ensure that there's a space between the check box and the text. Could we add something to our default CSS for this?

@black-desk
Copy link
Contributor Author

black-desk commented Jun 24, 2022

the screenshots I show in #8142 (comment) had apply my own css:

input[type="checkbox"] {
        margin-right:0;
        margin-left: -1.5em;
        width: 1.5em;
}

the default situation is weird:

图片

because the checkbox is part of the <p>, and pandoc's css set:

ul.task-list{list-style: none;}

:(

but in my option of view, the checkbox is some kind of ::marker which should be a sibling of <p>

图片


Update:

cmark-gfm's html output of

- [ ] aaa

  aaa

  aaa

is

图片

md render api of github return:

图片

so this is a bug of pandoc's tasklist ext :)


by the way github's css here:

图片

@jgm
Copy link
Owner

jgm commented Jun 24, 2022

OK, that makes sense. Maybe we should have a separate issue (since this PR is now closed) suggesting making the checkbox a sibling of the block-level content, and suggesting some default CSS for it?

@black-desk
Copy link
Contributor Author

OK, that makes sense. Maybe we should have a separate issue (since this PR is now closed) suggesting making the checkbox a sibling of the block-level content, and suggesting some default CSS for it?

done, #8151

mb21 added a commit that referenced this pull request Jul 4, 2022
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.

4 participants