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

Added task list ability to GFM parser. #442

Closed
wants to merge 2 commits into from
Closed

Added task list ability to GFM parser. #442

wants to merge 2 commits into from

Conversation

@adwylie
Copy link
Contributor

@adwylie adwylie commented Jul 14, 2017

Followed most of the suggestions in #172. Never written Ruby before though so it may be a bit sloppy.

Currently renders as li disc followed by the checkbox, was assuming there'd be css to change list-style-type to none if the checkbox exists. If not I can change it.

Thanks, Andrew.

Copy link
Owner

@gettalong gettalong left a comment

Thanks for your pull request! Please see the line comments for needed changes.

And please also add tests into the test/testcases_gfm/ directory to verify that everything works as expected.

Thanks!

# elements where necessary (as well as applying classes to the ul/ol and li elements).
def parse_list
super
@tree.children.each do |element|

This comment has been minimized.

@gettalong

gettalong Sep 3, 2017
Owner

Using @tree.children.each would mean that, e.g., parsing the third list involves checking the first two lists. It would be better to restrict the processing to the list at hand which saves time.

def parse_list
super
@tree.children.each do |element|
if [:ul, :ol].include? element.type

This comment has been minimized.

@gettalong

gettalong Sep 3, 2017
Owner

Please use parentheses around method arguments.

/\[ \]\s+/, '<input type="checkbox" class="task-list-item-checkbox" disabled="disabled" />')
unchecked = li.children[0].children[0].value.gsub!(
/\[x\]\s+/i, '<input type="checkbox" class="task-list-item-checkbox" disabled="disabled" checked="checked" />')
is_tasklist = (checked != nil or unchecked != nil)

This comment has been minimized.

@gettalong

gettalong Sep 3, 2017
Owner

Please use || instead of or; and checked.nil? as this avoids a method call.

Since is_tasklist is also used outside this loop, you need to use is_tasklist ||= ... since otherwise only the state of the last list item determines the status (which is probably not wanted).

/\[x\]\s+/i, '<input type="checkbox" class="task-list-item-checkbox" disabled="disabled" checked="checked" />')
is_tasklist = (checked != nil or unchecked != nil)
if is_tasklist
li.attr[:class] = 'task-list-item'

This comment has been minimized.

@gettalong

gettalong Sep 3, 2017
Owner

  • Please use the if-modifier syntax syntax for this since it is a short line and easy to understand.
  • Use attr['class'] since the keys are strings, not symbols.

All this also applies to line 176.

@adwylie
Copy link
Contributor Author

@adwylie adwylie commented Sep 22, 2017

Hey. I've addressed all of the comments and added some tests. Thanks!

@gettalong
Copy link
Owner

@gettalong gettalong commented Sep 22, 2017

Thanks - I'm currently rather busy but will have a look at soon as possible!

@gettalong gettalong self-assigned this Nov 26, 2017
@gettalong
Copy link
Owner

@gettalong gettalong commented Nov 26, 2017

Thanks for your contribution - I have squashed the commits and integrated your code.

@gettalong gettalong closed this Nov 26, 2017
@robbiejaeger
Copy link

@robbiejaeger robbiejaeger commented Oct 12, 2018

@adwylie @gettalong Thanks for this added feature!

Curious what the decision was for making the checkbox disabled by default?

@gettalong
Copy link
Owner

@gettalong gettalong commented Oct 12, 2018

@robbiejaeger kramdown generates static content, so if the input would be clickable the visible state would change but nothing else.

@robbiejaeger
Copy link

@robbiejaeger robbiejaeger commented Oct 12, 2018

Makes sense - thanks for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants