-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Propagate changes of md-rendered checkboxes to the server #14258
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14258 +/- ##
==========================================
- Coverage 41.97% 41.87% -0.11%
==========================================
Files 735 742 +7
Lines 78933 79324 +391
==========================================
+ Hits 33132 33214 +82
- Misses 40346 40647 +301
- Partials 5455 5463 +8
Continue to review full report at Codecov.
|
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.
Thank you! 🎉
Two edge cases to consider:
-
Your markdown <-> checkbox matching fails for pathologic markdown like the following:
[x] - [ ] foobar
This should be fixable with a stricter regex like
^- \[[ x]\]
-
The checkboxes are still interactive when user can't edit a comment. For accessibility I'd prefer if these checkboxes remain disabled. You could check if a sibling matching
.comment-container .comment-header-right .menu .item.edit-content
exists, or (cleaner) add a data-attr to the comment indicating if it is editable. For the cleaner solution, edittemplates/repo/issue/view_content/comments.tmpl
with a similar check to{{if or .ctx.Permission.IsAdmin .IsCommentPoster .ctx.HasIssuesOrPullsWritePermission}}
as I showed in #14258 (comment) the regexp route cannot work - we need to store the index in the raw markdown of the checkbox as described here: #14258 (comment) (and some kind of hash of the raw markdown too to ensure it hasn't changed in the intervening period) then switch the checkbox that way.
Agreed - these should be disabled if the user does not have permission to edit. We can pass in the permission check as part of render config to goldmark to get it to render actual forms for the checkboxes - although getting that past the sanitizer as currently constituted could be difficult - or just get javascript to enable them - by passing in the permission flag as part of the render. |
|
I'm still up for this, but having a lot on my plate right now. I'll get it done when I find the time to read up on go a little, so I don't just blindly apply zeripath's diff (thanks for that) but know how it works. Shouldn't be that long. If anyone's willing to do it right away, go ahead. Will report back here once I start working on it again |
Markdown rendered checkboxes are now enabled/clickable in comment blocks, markdown is changed accordingly and the raw comment is updated on the server.
Attaches
change
handlers to the markdown rendered checkboxes.When a checkbox value changes, the corresponding [ ] or [x] in the markdown string is set accordingly and sent to the server.
On success it updates the raw-content on error it resets the checkbox to its original value.
While a request is in-flight, all checkboxes within the same markdown block are disabled.
#7097