tgrosinger / tw5-checklist

Simple checklist widget for TiddlyWiki5:
https://tgrosinger.github.io/tw5-checklist/
51 stars 12 forks source link

New options to sort items in checklist alphabetically #40

Closed chanilino closed 6 years ago

chanilino commented 7 years ago

I have addded a new option in configuration, and implemented a feature to allow sort alphabetically items in checklist. It is tested and working.

I will love you add this feature in your next release!

Thank you!

tgrosinger commented 7 years ago

@chanilino, thank you so much for the contribution to this plugin. It sounds like a great feature and I will go though the code review very soon.

tgrosinger commented 7 years ago

Hi @chanilino, thanks again for your pull request. I really appreciate how you stuck with the style of the rest of the code base. I got a chance to test out your change and found a couple of bugs I was wondering if you could address.

Let me know if you have any questions about reproducing these issues or if I can help with solving them. Thanks again. The feature sounds like a great idea!

chanilino commented 7 years ago

Hi @tgrosinger are your testing the last commit (fe440254a9ce0b42ba9fc5375c79882b75f3ffdb)?

I only can reproduce one problem: "doesn't look like items are being sorted when adding them to the list, only when a check box is interacted with.". That's right it is not being considered in my implementation.

I will work on it and test more intensively this feature. I will commit soon an improved version!!

Thank you,

chanilino commented 7 years ago

I have just commit (861df5ec96f24bf240719cf7a679c2f9e2137a70) solving the issue of new items. I think now it works ok! If you see any problem, report it!

This commit also include a refactor of managing of events with more generic and reusable code.

I wait for your comments.

tgrosinger commented 7 years ago

I believe I was looking at the most recent commit, but I have updated to look at the new commit that you added. For some reason this version does not seem to be able to have more than three items in the list, adding a new one will bump off the bottom element.

Can I ask how you are testing this? There are instructions here on how to build a TiddlyWiki edition containing your changes. Let me know if you have any questions I can help with.

chanilino commented 6 years ago

I was developing directly in the browser, folowing this guide

I am developing using firefox, and it works. Perhaps it is a problem of browser compatibility. I will test using node. When you test with node, which browser do you use?

I will work a little more on this issue, after the weekend.

chanilino commented 6 years ago

I have just found the bug inserting new elements. I will work on it.

chanilino commented 6 years ago

Ok, the commit (d7fc000) solves the problem inserting elements!!

Now I think it is woking ok! Here I add an explanation how it works with configuration. Sort items feature changes its behaviour with the Rearrange checked items configuration. Following table tries to explain the behaviour:

Sort list alphabetically Rearrange checked items Expected behaviour
checked checked We have first unchecked items sorted alphabetically, after we put checked items sorted alphabetically
checked unchecked We have items sorted alphabetically and it is independent if it is checked or unchecked item
unchecked checked We have first unchecked items without alphabetical sorting, after we put checked items without alphabetical sorting
unchecked unchecked the items are in order of insertion
tgrosinger commented 6 years ago

@chanilino I am very sorry it took me so long to merge this. It looks great, thank you for your contribution!