-
-
Notifications
You must be signed in to change notification settings - Fork 268
Feature: tasklist with clickable label #2819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
36b39de to
756d2f5
Compare
|
I will take a look when I get some time. I will state that I appreciate you putting it under a switch, as that at least helps its chances. I am curious why the stash is needed. If you could elaborate further on that, it would help. While I won't promise acceptance right now, I will try to approach this with an open mind. It may require changes if the behavior in some cases is deemed to be awkward. I know right now I have concerns about certain content nested under list times, and I know you mentioned possibly leaving those out. Does this change leave them out? Or did we want to explore how well they work included first? |
|
I'm not sure how you were running ruff, but please use |
|
For proper HTML, it seems the label element is inline and should not contain block elements. This also greatly limits how it should be used when speaking about placing all the content of a list item under it. |
|
Hello, Thanks for the feedback. So, the Right now, I do not test for what's inside the However, regarding your last comment on the label element having to be inline and that it should not contain block element, could we define some test cases for this? I have a hard time figuring out how markdown would look for this. - [ ] Maybe something like this?
- [ ] ```
test code
on several
lines
```This renders as inline code (not multiline). Also, it seems like the html standard for the label element is quite lax regarding what could be part of it, from
All those could be elements embedded inside the label. |
|
For the linter, I have an error when running Probably a config error on my side, I'll have to investigate. EDIT: I fixed it. Not sure what happened in my env, but it runs properly now. Usually, I use ruff directly in vscode or in my terminal (as its already installed on my system). I thought it would run properly when I saw the |
|
Another detail, regarding the |
|
Yes, I finally fixed it in my env! Thanks for the info. I had a weird issue (because i'm used to use |
This will give loose lists (with paragraphs): Regarding the following: If you have SuperFences enabled, it requires something like this: This is because of the gymnastics we have to perform to properly identify fenced code in all situations in the Python Markdown parser. |
|
On a side note, it appears when lists are loose, that I also noticed when I'm still a bit confused about the stash. I feel like it shouldn't be needed. I'll have to investigate. |
|
Please give me an example where To be honest, I probably should have created real etree elements instead of just shoving text HTML in...that's probably the better solution. I did this so long ago (maybe 10 years ago), that's probably why I did it subpar with plain text. |
|
Never mind. I see now, you were simply moving the stash call. Don't worry about it then. If this is accepted (or even if it isn't), I may clean that up. |
|
For loose lists, I believe it works as expected, it's already one of the test cases. I did not try with SuperFences, I'll run it and see how it goes. Will it be enough to add |
|
Yeah. That's the old style tests, but it should work. I need to move tasklists tests over to the new approach, but I won't force you to do that. |
|
When However, I can add a proper label to this too and clean this a bit. This would allow to create clickable labels even when the style is not customised. |
|
For the stash, yeah, it's a simple move. That's also why I put these in their own commits. Would be easier to cherry-pick those if needed. |
|
What's the new approach? I can have a look and try my hand at it! I have known good tests there so I can port that if needed. If I have to put my hands in tasklist, I might as well upgrade what needs an upgrade! |
|
I guess thinking about the implementation more. The reason I probably used the stash was that no content was under the label. There was no concern about further post-processing, but now that you are moving content under the label, we should not be putting it in the stash as that will hide the content, and some things may be expecting some post-processing. We would likely need to attach proper etree elements. |
| ' disabled' if not self.clickable_checkbox else '', | ||
| ' checked' if state.lower() == 'x' else '') + | ||
| '<span class="task-list-indicator"></span></span>' | ||
| ) + match.group('line') + self.md.htmlStash.store("</label>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There, I break up the stash, so the matched group for the rest of the line gets properly rendered
|
That is why i break up the stash in two parts for the clickable label part of the code. Line 76, I put a comment there. |
|
I agree though it's not very clean. |
|
Yeah, fenced code is handles weird. It needs to be handled as a preprocessor, which is before there is an HTML tree. So they are found and inserted as placeholders. I believe when this is processing things, it's just seeing a placeholder. |
|
This is one of the cumbersome things about Python Markdown, and why it may be more effort to achieve what you're looking for than it is really worth. |


Hello,
Following our conversation in #2818, I took a stab at this. Let me know what you think about the proposed changes.
I'm not sure how to properly format the text, ruff wanted to do some changes in other places than I've touched (mainly replacing
'with"). Let me know if it's ok to run it and make some changes in places I didn't touch. For now, I didn't use it for clarity of the changes proposed.For the changelist:
get_checkboxfunction to the classTasklistTreeprocessor, to simplify the call to the function.md.htmlStash.storecall inside theget_checkboxmethod.clickable_labeloption and set it so that when in use, the label include whatever markdown code (or html) is included in the task item.I didn't change the documentation yet, and probably there are a couple of formatting issue, hence the draft on this pull request.