Proper coloring of items with DONE state

[schoettl]

This closes #16

@munen In case you know at the first glance where to get todoKeywordSet from, I'd be happy about a hint. Otherwise I'll figure it out, shouldn't be too hard.

[munen]

@schoettl You can get the todoKeywordSets as in this discussion: #154 (review)

A single headline doesn't have a todoKeywordSet. Those are set on a buffer level with these in-buffer settings: https://github.com/200ok-ch/organice#in-buffer-settings

[schoettl]

Thanks, I got it :)

I hope this fix (especially list.every(... list2.includes(...))) does not slow down the rendering too much. If JS is smart, it would compute it only once for the same input...

Or should I add a line FIXME: comment with a note on speed optimization?

[munen]

Yes, please add the FIXME. I’ll add memoization when I get to the PR^^

[munen]

From looking on the diff on my phone: This looks very nice. I like you implemented it in a functional way. This will be easy to memoize, too!

I’ll probably also add an acceptance test as safeguard against regressions before merging, because it’s a user visible feature.

[schoettl]

That sounds good!

I like you implemented it in a functional way. This will be easy to memoize, too!

... coming from Haskell in my last projects :)

I will watch your changes, I have to learn about acceptance and integration tests...

[munen]

... coming from Haskell in my last projects :)

It shows! The best JS devs I know in person or have seen in FOSS projects are the ones with some background in proper FP languages - often from Clojure or Haskell. Happy to have you here^^

Regarding your proposed FIXME: I really like that you're thinking of performance! And in the other search feature, we likely will have to think of it some time. But back to this feature: When I was commenting before, I was only on my phone and thought too quickly. For the particular function that you wrote, memoization would be overkill. The function you wrote checks if a value is a member of a small set. todoKeywordSets will never be many. By default it's two (TODO, DONE) and even if people add some, it'll be a handful, 10 or let's even say 20 (which I have never seen even in other PM apps). Memoization adds some overhead as it needs not only to remember values for inputs, but it also needs to look up based on what criteria the value should be looked up. Hence, I think that firstly your function will always be proper fast whilst memoization might even introduce a small penalty. To be fair, before I started to use my brain, I actually looked into the React profiler to benchmark your code against master. And there's no difference to be seen, because your function is proper fast(;

However, I did the other thinks we talked about - I made a refactoring, documented the feature in the sample.org and and wrote an integration test.

If you were curious on how I would have done the memoization: If the component would have been a functional component, we could have used a React feature called hooks - specifically the useMemo hook. Since this is not a functional component, I would have used lodashs' memoize function.

Really great feature, thank you very much for building it! 🙏 🙇

[munen]

I have updated the changelog with the new feature and attribution: d7db53c

[schoettl]

Thanks for the information. Perfect, that it doesn't compromise performance! We can remove the comment when we come to that place again. In the meantime, I'm confident that nobody else will work on this and adds unnecessary complexity :D

[munen]

Perfect, that it doesn't compromise performance!

🚀

We can remove the comment when we come to that place again.

👍