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

Add TODO list support for Blackfriday #2296

Merged
merged 6 commits into from
Sep 9, 2016
Merged

Conversation

bep
Copy link
Member

@bep bep commented Jul 21, 2016

Fixes #2269

  • Basic support
  • A flag to turn it off
  • Add a way to style the list bullet away
  • A test
  • Some documentation

@bep bep changed the title Add TODO list support for Blackfriday WIP: Add TODO list support for Blackfriday Jul 21, 2016
@bep
Copy link
Member Author

bep commented Jul 21, 2016

screen shot 2016-07-21 at 18 46 27

But we need to add a CSS-class somewhere so it is possible to hide the list bullet (as GitHub does).

/cc @phtan

@bep
Copy link
Member Author

bep commented Jul 21, 2016

This is how GitHub does it for the li-item:

class="task-list-item enabled"

@bep bep changed the title WIP: Add TODO list support for Blackfriday Add TODO list support for Blackfriday Jul 22, 2016
@bep bep added this to the v0.17 milestone Aug 5, 2016
@bep bep assigned spf13 Aug 8, 2016
@bep
Copy link
Member Author

bep commented Sep 8, 2016

I have rebased this against master now and I suspect the tests will pass.

I'm planning to implement this for the MMark renderer, too, but I kind of need this code to do that.

@digitalcraftsman @moorereason I know this works, but do you have any complaints? Good to merge?

@moorereason
Copy link
Contributor

Any reason why you set checked and disabled to empty strings? I would use:

<input type="checkbox" checked disabled class="task-list-item">

@bep
Copy link
Member Author

bep commented Sep 8, 2016

Any reason why you set checked and disabled to empty strings?

If you followed the looooong discussion in the related issue you will see that the core part of the implementation is borrowed from @shurcooL ... Knowing that he writes compilers for fun (gopherjs), I'm pretty sure he is correct. And reading the spec (https://www.w3.org/TR/html-markup/syntax.html#syntax-attributes), I don't see any "attribute without value" option. It probably works in most (all?) browsers, but isn't entirely correct.

@moorereason
Copy link
Contributor

@dmitshur
Copy link

dmitshur commented Sep 8, 2016

If you followed the looooong discussion in the related issue you will see that the core part of the implementation is borrowed from @shurcooL ... Knowing that he writes compilers for fun (gopherjs), I'm pretty sure he is correct.

Thanks, but I would recommend reviewing that code from scratch. I wrote it a while ago and decisions/research done back then may not apply or still be valid today. It also highly depends on the context where it's used (version of HTML, etc.).

Any reason why you set checked and disabled to empty strings?

There was no good reason, it was just one of many ways of doing it. I think your suggestion is valid and makes sense. But I haven't looked at the specifics of HTML/XHTML versions hugo uses, its browser support, etc.

@bep
Copy link
Member Author

bep commented Sep 8, 2016

Both variants is fine, then. I chose the one with the least amount of work.

`checked` and `disabled` doesn't have to have an empty value.

See gohugoio#2296
@bep
Copy link
Member Author

bep commented Sep 9, 2016

@moorereason I have simplified the HTML attributes re. your comment, does it then look generally OK?

@shurcooL if people use your GitHub renderer (or whatever it is named), you should take another look at your TODO-list implementation. A HTML list without any CSS classes isn't very easy to style.

@digitalcraftsman
Copy link
Member

The commit looks fine. I agree with @moorereason that the empty strings can be removed because the w3c spec handles them equivalent:

Note that empty attribute syntax is exactly equivalent to specifying the empty string as the value for the attribute, as in the following example.

<input disabled="">

@bep bep merged commit eaf2f9b into gohugoio:master Sep 9, 2016
@bep bep deleted the tastklist-bf branch September 9, 2016 11:08
@dmitshur
Copy link

dmitshur commented Sep 10, 2016

@shurcooL if people use your GitHub renderer (or whatever it is named), you should take another look at your TODO-list implementation. A HTML list without any CSS classes isn't very easy to style.

The goal of the github_flavored_markdown package is to render GitHub Flavored Markdown, generating HTML and providing the CSS that displays it. The user isn't meant to customize it in any way – that's what blackfriday and its custom renderers and options are for. If you think there's a bug, you should open an issue in that repo.

@bep
Copy link
Member Author

bep commented Sep 10, 2016

If you think there's a bug, you should open an issue in that repo.

I don't use that repo, just giving you a heads up.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TODO lists support for Blackfriday (*.md files)
5 participants