Closed batpad closed 7 months ago
Thanks for submitting your first pull request! You are awesome! :hugs:
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! :tada:
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!
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 (https://github.com/executablebooks/mystmd/issues/1037).
Congrats on your first merged pull request in this project! :tada:
Thank you for contributing, we are very proud of you! :heart:
Thanks for your work on this @batpad -- I will aim to get a release out today.
@rowanc1 ha, the typeof
check looks a lot better than what I had :) - thank you SO much for the quick fix and merge here.
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
Was just looking at https://github.com/executablebooks/jupyterlab-myst/commit/2b288cdd08394575aa2d87c0bc55caed9f3254a4 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 thechecked
variable isfalsey
- so that would be true fornull
orundefined
, 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.