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

GFM compliance for tasks lists #1250

Merged
merged 4 commits into from May 8, 2018

Conversation

Projects
None yet
4 participants
@tomtheisen

tomtheisen commented May 3, 2018

Marked version: 0.3.19

Markdown flavor: GitHub Flavored Markdown

Fixes #107
Fixes #419
Fixes #170
Fixes #689
Fixes #587

Description

This adds task list items support as specified in GFM. These are the checkboxes you can make in bulleted lists.

I did do some funny stuff in the gfm test runner. In particular all the other gfm tests use the xhtml rendering option. But for some reason, (probably not a good one) the checkbox rendering in the gfm spec is not xhtml. So, I added a an xhtml attribute to the test spec json for those tests. It seems pretty janky. Maybe someone can think of a better way of handling it.

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR
Tom Theisen
+ (checked ? 'checked="" ' : '')
+ 'disabled="" type="checkbox"'
+ (this.options.xhtml ? ' /' : '')
+ '>';

This comment has been minimized.

@UziTech

UziTech May 3, 2018

Member

from the specs it looks like there should be a space after >

@@ -15,7 +15,8 @@ Messenger.prototype.test = function(spec, section, ignore) {
var shouldFail = ~ignore.indexOf(spec.example);
it('should ' + (shouldFail ? 'fail' : 'pass') + ' example ' + spec.example, function() {
var expected = spec.html;
var actual = marked(spec.markdown, { headerIds: false, xhtml: true });
var usexhtml = typeof spec.xhtml === 'boolean' ? spec.xhtml : true;
var actual = marked(spec.markdown, { headerIds: false, xhtml: usexhtml });

This comment has been minimized.

@UziTech

UziTech May 3, 2018

Member

It looks like just setting the xhtml option to false for all gfm the tests works

This comment has been minimized.

@joshbruce

joshbruce May 3, 2018

Member

I think CommonMark uses XHTML maybe the GFM extensions don't need XHTML. (I do think there's something to be said for the attempted solution - putting the options in the JSON - but not sure we've hit a point of actually needing it.)

This comment has been minimized.

@tomtheisen

tomtheisen May 3, 2018

I'll just turn off xhtml altogether for gfm tests, and remove this spec property.

@@ -363,10 +365,20 @@ Lexer.prototype.token = function(src, top) {
if (!loose) loose = next;
}
// Check for task list items
istask = /^\[[ xX]\]/.test(item);

This comment has been minimized.

@UziTech

UziTech May 3, 2018

Member

a space seems to be required after [ ] other wise it is rendered like text

with space:

  • test

without space:

  • [x]test

This comment has been minimized.

@tomtheisen

tomtheisen May 3, 2018

I'll add this space to the pattern, along with the corresponding change below.

ischecked = undefined;
if (istask) {
ischecked = item[1] !== ' ';
item = item.replace(/^\[[ xX]\] */, '');

This comment has been minimized.

@UziTech

UziTech May 3, 2018

Member
- /^\[[ xX]\] */
+ /^\[[ xX]\] +/

since at least one space is required

@tomtheisen

This comment has been minimized.

tomtheisen commented May 4, 2018

I've addressed a few things.

  1. xhtml option is hardcoded to false for gfm tests.
  2. Space is required following task markdown.
  3. Task checkbox html is rendered with a space following it.

There are no other outstanding concerns to my knowledge.

@UziTech

UziTech approved these changes May 4, 2018

Tom, you are on a roll 💯

@tomtheisen

This comment has been minimized.

tomtheisen commented May 4, 2018

I'm on vacation this week, and I resolved not to work on work, but I have to work on something!

@styfle styfle added this to the 0.4.0 - No known defects milestone May 8, 2018

@styfle styfle changed the title from GFM compliance for tasks to GFM compliance for tasks lists May 8, 2018

@styfle

This comment has been minimized.

Member

styfle commented May 8, 2018

@tomtheisen This is huge! 🏋️
Thanks for implementing this! 🎉

@styfle styfle merged commit 42c3915 into markedjs:master May 8, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
WIP ready for review
Details
security/snyk - package.json No known issues
Details

@joshbruce joshbruce referenced this pull request Jun 13, 2018

Closed

Github Task Lists #587

@dorokei dorokei referenced this pull request Oct 20, 2018

Closed

GFM task list #35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment