FIX: use not checked instead of == to hopefully make the linter happy?

[batpad]

Was just looking at 2b288cd and it looks like a nice fix, except it makes the linter unhappy because it does not like the == operator.

For various reasons, most Javascript style guides will discourage the use of == since the rules for fuzzy equality in javascript can be a bit obscure. Here I try changing that to a !checked which should do the same thing - i.e. check if the checked variable is falsey - so that would be true for null or undefined, which I think was the intention behind the == ? Am not fully sure how to test this, but I'll try - if anyone is able to verify though, I do hope this both fixes the issue and makes the linter happy.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

[github-actions]

👈 Launch a Binder on branch batpad/jupyterlab-myst/fix/list-checkboxes

[batpad]

Hm, so I see why the == null there was a clever hack. I pushed another commit that checks that checked !== true and also !== false, which should now work correctly, and also make the linter happy.

While this works for both cases, where it's a regular list item, and where prefixed with a [ ] and it needs to be a "Task List Item", I would highly recommend refactoring this a bit.

The problem seems to be:

A property called checked is set on the node, based on a regex checking for whether the [ ] checkbox in a list item is in the checked state ([x]) or unchecked state ([ ]). This is used by the TaskListItem to decide whether to display the input checkbox in a checked or unchecked state: https://github.com/executablebooks/jupyterlab-myst/blob/main/src/components/listItem.tsx#L16 .

The problem as I see it is that we try and use this same checked property to decide whether the item is a TaskListItem or a regular ListItem here: https://github.com/executablebooks/jupyterlab-myst/blob/main/src/components/listItem.tsx#L48 - so, if it is a TaskListItem, checked will always be either true or false, and if it is a regular list item, checked will be undefined (or null?). I feel like it would be better to use a separate property to denote whether a node is a Task List Item or a regular List Item, instead of trying to overload the usage of the checked variable, which denotes whether the checkbox is checked or not.

I'd advocate for adding something like is_task_item as a property on the node, based on a regular expression that just checks whether the string contains either [ ] or [x], which denotes it as a Task List Item, and then use that to decide whether to render ListItem or TaskListItem instead of this slightly weird check on the the value of checked.

However, I'd also be extremely happy if it were possible to make a release with this fix (or a version of it) as currently all list formatting is broken 😢 - if the above refactor sounds like a good idea, am happy to give it a shot, but would rather it not hold up this fix.

Thank you again for this project!

[rowanc1]

I have moved the logic around slightly to have the checked flag be explicitly looking for a boolean. The - [ ] logic in the regexp is only to update the markdown source. Changing the node type to a task item rather than a list is sensible. I opened an upstream issue for that (executablebooks/mystmd#1037).

[welcome]

Congrats on your first merged pull request in this project! 🎉


Thank you for contributing, we are very proud of you! ❤️

[rowanc1]

Thanks for your work on this @batpad -- I will aim to get a release out today.

[batpad]

@rowanc1 ha, the typeof check looks a lot better than what I had :) - thank you SO much for the quick fix and merge here.

[rowanc1]

We will aim to get the release out in the next few days - our automated release infrastructure is broken at the moment. :(
https://github.com/executablebooks/jupyterlab-myst/actions/runs/8470399758

cc @agoose77