On 01/26/2015 06:37 PM, Jan Heylen wrote:
Hi,
I've been working on a change that a user would allow to open multiple
comments on a commit, and 'post' (save) them all at once to the
kallithea database. Currently this is client side only change, but
with the proper changes on the server side end, this could result in a
one-mail-per-commit-review instead of a one-mail-per-comment setup
now.
But the main reason I was working on this is the fact that if you add
2 or more comments to a commit, and you choose to save them as preview
first (you are still reviewing the rest of the commit), you have to go
over all the comments and press the save button.
Where I want to take this is even one step further:
We have now:
For inline comments: save - cancel - preview
And below the commit: approved/rejected/close/... and comment - preview
I would change this to:
For inline comments: comment - cancel
Below the commit: submit all comments - cancel all
I think it could be annoying if we don't have a comment/save/submit
button next to the inline comment. But I guess the text on the button
should change to something with "all comments".
The inline 'comment' buttons would actually do what is now a preview
(and may still indicate comment preview), cancel remains the same.
The 'submit all comments' would do an AJAXPost of all the comments (no
backend change required) at once.
I don't see the point in rst/md markup of comments. People should
comment on content, not spend time making it look fancy. We only saw
people being annoyed by their code snippets being obfuscated by the
markup processing. We use
https://bitbucket.org/Unity-Technologies/kallithea/commits/09286e5ca064de6930d5bdefb9df6708eda19976
and do thus not really have any need for preview. We could perhaps move
in that direction here upstream and avoid the problem you mention.
What do you think?
+1 to the general idea and your thoughts.
Also, we should introduce a "don't navigate away from this page with
pending comments" before we encourage users to not commit immediately.
I've made a little POC patch that does the submit-all-open-comments
thing, it does it for open comments for know, but probably not that
difficult (and even better, it won't require the remove-class hack
probably) to change that to a for all preview comments way of working.
I'll inline the POC patch for your reference, but forgive me my basic
javascript knowledge...
regards,
Jan
the patch:
diff -r bfc304687f1c kallithea/public/js/base.js
--- a/kallithea/public/js/base.js Wed Jan 21 17:35:11 2015 +0100
+++ b/kallithea/public/js/base.js Mon Jan 26 18:36:00 2015 +0100
@@ -667,6 +667,9 @@
$form.submit(function(e){
e.preventDefault();
+ if(!$tr.hasClass('form-open')){
+ return;
+ }
if(lineno === undefined){
alert('Error submitting, line ' + lineno + ' not found.');
return;
@@ -694,6 +697,10 @@
'line': lineno
};
ajaxPOST(submit_url, postData, success);
+ //proof of concept, preemptive remove the form from the open
list, not good, but only for POC
+ $tr.removeClass('form-open');
+ var allOpenForms = $( '.form-open' ).next();
+ allOpenForms.find( ".inline-form" ).submit();
});
$('#preview-btn_'+lineno).click(function(e){
It is not obvious to me what is going on with this form-open. Or ... you
are recursing and using this to make sure it terminates?
I would guess you could use something like
$('code-difftable form').each(function(){$(this).submit();});
or perhaps even
$('code-difftable form').submit();
But in the next step you will submit multiple comments in the same api
call. That means you either have to do it with javascript or change the
whole diff to be one form. This PoC might thus not be step towards a
real solution.
/Mads
_______________________________________________
kallithea-general mailing list
[email protected]
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
