search

deckar01 / task_list

Markdown Task List feature components
MIT License
9 stars 7 forks source link

Pass lineNumber and lineSource in changed event #23

Closed fatihacet closed 5 years ago

fatihacet commented 5 years ago

@deckar01 would you mind taking a look at it?

These changes are needed for us in GitLab for this MR.

I am working on adding the tests. Meanwhile, please give it a try and let me know what you think about it. Thanks 👍

deckar01 commented 5 years ago

Adding this info to the event would be safe for a minor version release.

If you are planning on resolving conflicts on the back end I would consider applying the check on the back end even when there isn't a conflict. Most of the bugs that I have fixed in this library are caused by the complex regex used to parse the markdown being different than the markdown parser on the back end. Walking the CommonMarker AST would be much more robust. You could grab the check index out of the tasklist:change event, cancel the event, and do the rest on the back end.

fatihacet commented 5 years ago

Adding this info to the event would be safe for a minor version release.

Thanks for the quick review. That's exactly what we want to do. Let's keep the changes minimum. 👍

Also Travis pipeline fails because of the error below. I don't what to do with it. Any idea?

deckar01-task_list@2.0.1 qunit /home/travis/build/deckar01/task_list phantomjs test/run-qunit.coffee http://localhost:4018/test/index.html Unable to open file '/dev/stderr'

deckar01 commented 5 years ago

A jQuery release broke compatibility with phantomjs. Using compatible versions fixed the issue. See #24 for more info. The tests are passing on master now.

fatihacet commented 5 years ago

@deckar01 rebased from master, thanks for fixing it 👍

deckar01 commented 5 years ago

I would like to hold off on merging this until you have logic on the back end that consumes it in case the requirements change. Is it OK if you continue using your fork during development of that GitLab feature?

fatihacet commented 5 years ago

Yeah sure. I think it makes sense to wait until the GitLab MR merged so we will have a final decision on the parameters etc. Meanwhile we can use my fork.