feat(#11): include input checkbox for task lists #12
Conversation
|
Hi @KyleKing! Yes, emoji test were broken in the master as tests download fresh HTML from djot syntax spec page (https://htmlpreview.github.io/?https://github.com/jgm/djot/blob/master/doc/syntax.html) which recently changed Regarding the approach - what do you think if input checkbox will be supported only in the rendering layer? I see drawback in the current approach as new AST for checkboxed-list can make maintenance harder because on one hand - this is new AST node but on the other hand - it must behave just as regular list and the only difference is in the presentation layer. I would suggest to add this feature only in the rendering layer by extending logic of The development flow is simple and you did everything right: |
Can't use because requires popping the class Key
This reverts commit ab4b5a7.
There was a problem hiding this comment.
Ready for review again! Feel free to make changes directly if easier than providing feedback
I also ran fuzz testing locally (go test -fuzz FuzzDjotTokenizer -fuzz FuzzDjotE2E github.com/sivukhin/godjot/djot_parser). I'm not sure how long to let it run, but it ran for two minutes after this fix without finding any new issues.
fuzz: elapsed: 0s, gathering baseline coverage: 0/699 completed
fuzz: elapsed: 0s, gathering baseline coverage: 699/699 completed, now fuzzing with 10 workers
fuzz: elapsed: 3s, execs: 165661 (55208/sec), new interesting: 13 (total: 712)
fuzz: elapsed: 6s, execs: 338017 (57444/sec), new interesting: 26 (total: 725)
...
fuzz: elapsed: 2m6s, execs: 5540830 (33608/sec), new interesting: 191 (total: 890)
^Cfuzz: elapsed: 2m7s, execs: 5553930 (23612/sec), new interesting: 191 (total: 890)
PASS
ok github.com/sivukhin/godjot/djot_parser 127.064s
djot_parser/djot_html.go
Outdated
| s.Writer.WriteString("\n") | ||
| s.Writer.WriteString("<input disabled=\"\" type=\"checkbox\"") | ||
| if class == CheckedTaskItemClass { | ||
| s.Writer.WriteString(" checked=\"\"") | ||
| } | ||
| s.Writer.WriteString("/>").WriteString("\n") | ||
| n(s.Node.Children[:1]) | ||
| s.Writer.WriteString("\n") |
There was a problem hiding this comment.
I finally had a window of time this afternoon and got the tests passing locally!
While this works, I'm not sure if the logic should be generalized (maybe not if this is the only self-closing tag) or for some way to pop Entries (e.g. removing the class from the AST for methods like the BlockNodeConverter). I'll defer to whatever you think is best
`go test -run=FuzzDjotE2E/326270ac6d2da709 github.com/sivukhin/godjot/djot_parser`
This is the first go project I've actually worked on, so any guidance would be appreciated! I've been running
make test lintlocally, but the Emoji test (35) fails and I think my changes are generally the right direction, but I'm not sure what to do to finish the changeFixes #11