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 task-list-item class to task list items #468

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicoburns
Copy link

@nicoburns nicoburns commented Sep 12, 2024

I am trying to render tasklists similar to how Github renders them. Unfortunately it is currently very difficult to differentiate between a tasklist and a regular list generated by comrak using CSS, which is leading to my renderer rendering both the checkboxes and the regular list bullets for tasklists.

Github works around this issue by adding a special class to the <ul> and the <li> elements, which allows list-style-type: none to be specified for tasklist list items (causing the regular bullets to be hidden). I have implemented this for the <li> element of task lists in this PR. Doing it for the <ul> as well is more difficult as it doesn't have a special AST node. And it is also not required to set the list-style-type style for the list items.

Illustration of the problem:

Screenshot 2024-09-12 at 13 21 06

Testing mixed lists:

  • Hello
  • World
  • Item
  • Item 2
  1. Numbered
  • Item
  1. Numbered

  2. Item

  3. Numbered

  • Bullet

  • Bullet

  1. Numbered

@digitalmoksha
Copy link
Sponsor Collaborator

Nice addition @nicoburns. I like the idea of having that class added, have thought about adding it myself - would save me from having to do it in post-processing. Though it might need to be behind a render option since it would change what the default GFM output is.

Having a class on the <ul> is useful. You could add a new property to NodeList that gets set when a task list item is added to the AST - its parent will be the <ul>. It should only be set on the first parent of a task list item, not any additional ancestors. It would be set as long as at least one item is a task item, even if it's a mixture of task list items and normal list items. This is also how GH behaves, you can inspect the example list below.

  • one
  • two
    • three

I like the class task-list (which is what we use), while GH uses contains-task-list

@digitalmoksha
Copy link
Sponsor Collaborator

One suggestion - it looks like you're making a specific choice between ordered, unordered, and task list. However, a task list can be either ordered or unordered. GH makes the distinction but doesn't display the numbers, while GitLab makes the distinction and does display the numbers.

So I would keep making the list either ordered or unordered, and then they either contain tasks or not.

@nicoburns
Copy link
Author

Yep! I thought I was being clever combining it into the type enum. Then realised the tests weren't passing 🤦 . Will rework this to use a separate property.

@nicoburns
Copy link
Author

Ok, done! And I've squashed it all down into one commit because the history was getting messy :)

@digitalmoksha
Copy link
Sponsor Collaborator

script/cibuild is failing because it's not passing the standard GFM spec. You're going to have to add a render option. But my naming_fu is failing me right now...

ListType::Bullet => {
self.output.write_all(b"<ul")?;
if nl.is_task_list {
self.output.write_all(b" class=\"contains-task-list\"")?;
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

I'm going to advocate again for task-list. This doesn't "contain" a task list, it "is" a task list. I don't think we need to follow GH on this. https://github.com/jgm/commonmark-hs also uses task-list.

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.

2 participants