thephpleague / commonmark

Highly-extensible PHP Markdown parser which fully supports the CommonMark and GFM specs.
https://commonmark.thephpleague.com
BSD 3-Clause "New" or "Revised" License
2.75k stars 195 forks source link

TaskList: Listener to update markdown #511

Open glensc opened 4 years ago

glensc commented 4 years ago

I was wondering if commonmark provided example for TaskListExtension how to:

  1. add javascript listener on click of task item
  2. modify markdown to handle the ticked state of the task item

this would make implementing full task list functionality easier for downstream projects.

glensc commented 4 years ago

I was thinking on high-level implementation would be something like:

  1. add internal id of each <input> so they could be identified globally
  2. add javascript click event handler to change checkbox value on page
  3. use the id and post "update" to backend server (action=tasklist_update&id=guid&state=on)
  4. backend server takes the id, modifies markdown, saves updated markdown
markhalliwell commented 4 years ago

I don't think IDs are necessary. In fact, GH doesn't use IDs at all. Instead, they base it on the order of first appearance/detection.

I used the following markdown to test what was being sent back and forth from the browser to GH's servers:

Cras justo odio, dapibus ac facilisis in, egestas eget quam. Duis mollis, est non commodo luctus, nisi erat porttitor ligula, eget lacinia odio sem nec elit. Duis mollis, est non commodo luctus, nisi erat porttitor ligula, eget lacinia odio sem nec elit.

Task List 0

- [ ] Item 0
- [x] Item 1
- [ ] Item 2

Curabitur blandit tempus porttitor. Aenean lacinia bibendum nulla sed consectetur. Nullam quis risus eget urna mollis ornare vel eu leo.

Task List 1

- [ ] Item 0
- [x] Item 1
- [ ] Item 2

Maecenas faucibus mollis interdum. Vestibulum id ligula porta felis euismod semper. Donec id elit non mi porta gravida at eget metus.

When dragging Item 1 from Task List 0 to the end of Task List 1, these are the parameters it sends in the POST request:

task_list_track: reordered
task_list_operation: {"operation":"move","src":[0,1],"dst":[1,3]}
task_list_key: issue

You can see that the source (src) is an indexed array, where the first numerical value is the task list index (0) and the second numerical value is the item index (1). The destination (dst) is in the same format.

How they actually perform the modification is anyone's guess, but I suspect they have a strict AST of their markdown which allows them to more easily move these items around at a whim.

As far as their FE sides of things, they implement their own <task-list> element and event listeners to detect when items are dragged and dropped.


This all being said, regardless of how this is implemented (which could be any number of ways), this is ultimately a specific server/application implementation.

I'm not entirely sure how this project can provide "documentation" for something that would be specifically unique to each implementation.

markhalliwell commented 4 years ago

I forgot to mention that the "checked" data looked like this:

task_list_track: checked:1
task_list_operation: {"operation":"check","position":[0,1],"checked":true}
task_list_key: issue
markhalliwell commented 4 years ago

From discussing this with @colinodell in slack, I think adding a DocumentParsedEvent in TaskListExtension would allow us to modify the AST prior to rendering.

However, after thinking about this more over the weekend I realize that this would only alter the final HTML output. It wouldn't be able to reconstruct the markdown, which would likely be a requirement, especially if this is in some sort of CMS like application that has to save the original markdown that was modified. This means that some components from https://github.com/thephpleague/html-to-markdown may be needed here.

Also, I'm not entirely sure how the "operations" ^^ should/would be queued. In theory, yes, we could easily use (abuse) the config since this extension doesn't have any; that, however, feels very wrong. These are (one time) operations/actions, not static configuration.


I honestly don't think there is any elegant solution here (yet). We likely won't know more about how to properly tackle this until, at a minimum, 2.0 is released. Even then, we'd also likely need to standardize the AST more and probably introduce components of https://github.com/thephpleague/html-to-markdown to effectively perform the necessary operations on the source.

glensc commented 4 years ago

Leaving aside how GitHub has implemented, I'm fairly certain that doesn't need to use an HTML converter if each checkbox has a unique id and there's a way to serialize AST back to markdown. then the unique id that's ticked gets send over the wire, you find respective checkbox element from AST, apply state change and that's it.

I haven't seen the AST in this project, so I might be overoptimistic due that :)

markhalliwell commented 4 years ago

Leaving aside how GitHub has implemented

Considering that the TaskListExtension is based solely on the GFM spec, I don't think we should ignore how GH implements its FE implementation. Clearly, they've put a lot of thought and development hours behind how this extension is supposed to operate. I think it would be wise to take note of and attempt to reverse engineer their FE implementation, not brush it aside and create something arbitrary based on a single use-case.

if each checkbox has a unique id and there's a way to serialize AST back to markdown

There's currently no reliable way AFAIK to convert the AST back into markdown (in this project specifically). That's part of the "components" I was referring to from https://github.com/thephpleague/html-to-markdown. We won't need to parse HTML during a DocumentParsedEvent as we're already dealing with the parsed markdown AST, it just needs to additionally be able to render it back down into markdown as well is all I'm saying (as well as HTML for the final output). Maybe drawing it out a bit will help explain this better:

                                              Markdown (source)
                                                     |
                                                 Parsed AST
                                      _______________|_________________
                                     /                                 \
Render Markdown (save altered source in CMS)                        Render HTML (output to browser)

then the unique id that's ticked gets send over the wire, you find respective checkbox element from AST, apply state change and that's it.

This "unique id" would work for this particular operation, but it wouldn't be able to handle the movement of items inside an existing list or between lists. This is something we'd likely want to support as well since it too is a very useful feature. Building on top of the first reply, GH has already determined that a simple two-point index-based coordinate system is sufficient. This allows it to:

  1. Perform relatively simple alterations based on the order of first appearance (check/uncheck/move)
  2. Avoids identifier mismatch when items are moved

I think the second point is likely the main reason they don't use identifiers as any identifier generated would likely be a result of the current index, which can change upon an item being moved; thus only causing confusion when the original identifier would be referencing a different item.

colinodell commented 4 years ago

This is related to #419 as both would require rendering the AST to Markdown

quarterdeck commented 3 years ago

This is kind of off topic as I'm not concerned about updating the source markdown but this is how I dealt with persisting the checkbox states in lieu of any official method.

document.querySelectorAll('input[type="checkbox"][disabled]').forEach((checkbox) => {
    checkbox.disabled = false; // enable checkbox (by default they are disabled)
    checkbox.id = btoa(checkbox.parentElement.textContent.trim()); // generate a unique id
    checkbox.checked = JSON.parse(localStorage.getItem(checkbox.id)); // set status of checkbox based on Local Storage value
    checkbox.addEventListener('change', (event) => { // store checkbox state when it is changed
      localStorage.setItem(event.target.id, event.target.checked);
    });
});

IDs will change if the text is changed but that's kind of what I want so you might have to fiddle around if you don't like this. You could identify checkboxes based on another method, like index position for instance as mentioned.