Merge pull request #7800 from Abijeet/bug-7281

Fixes the usability issues with the comment section delete and edit
This commit is contained in:
Morris Jobke 2018-04-11 15:27:12 +02:00 committed by GitHub
commit a18a853e68
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 243 additions and 105 deletions

View file

@ -13,55 +13,42 @@
}
#commentsTabView .newCommentForm {
position: relative;
margin-bottom: 20px;
margin-left: 36px;
}
#commentsTabView .newCommentForm .message {
width: calc(100% - 81px); /* 36 (left margin) + 30 (right padding) + 15 (right padding of surrounding box) */
margin-left: 36px;
padding-right: 30px;
display: block;
/* width = 100% - (width of submit button (44px) + margin (3px) + inline-block gap) */
width: calc(100% - 52px);
display: inline-block;
}
#commentsTabView .newCommentForm .submit {
position: absolute;
bottom: 0px;
right: 8px;
width: 30px;
width: 44px;
margin: 0;
padding: 7px 9px;
padding: 13px;
background-color: transparent;
border: none;
opacity: .3;
vertical-align: bottom;
}
#commentsTabView .deleteLoading {
float: right;
padding: 14px;
vertical-align: middle;
}
#commentsTabView .submitLoading {
vertical-align: bottom;
display: inline-block;
padding: 14px;
}
#commentsTabView .newCommentForm .submit:not(:disabled):hover,
#commentsTabView .newCommentForm .submit:not(:disabled):focus {
opacity: 1;
}
#commentsTabView .newCommentForm .submitLoading {
background-position: left;
/* Match rules for '#commentsTabView .newCommentForm .submit' to place the
loading icon at the same position as the confirm icon */
position: absolute;
bottom: 0px;
right: 8px;
width: 30px;
margin: 0;
padding: 7px 9px;
/* Match rules for 'input[type="submit"]' to place the loading icon at the
same position as the confirm icon */
min-height: 34px;
box-sizing: border-box;
}
#commentsTabView .newCommentForm .cancel {
margin-right: 6px;
}
#commentsTabView .newCommentForm div.message {
resize: none;
}
@ -73,11 +60,16 @@
#commentsTabView .comment {
position: relative;
margin-bottom: 30px;
/** padding bottom is little more so that the top and bottom gap look uniform **/
padding: 10px 0px 15px;
word-wrap: break-word;
overflow-wrap: break-word;
}
#commentsTabView .comments .comment {
border-top: 1px solid $color-border;
}
#commentsTabView .comment .avatar,
.atwho-view-ul * .avatar{
width: 32px;
@ -123,19 +115,22 @@
background: -o-linear-gradient(rgba(255,255,255,0), rgba(255,255,255,1));
background: -ms-linear-gradient(rgba(255,255,255,0), rgba(255,255,255,1));
background: linear-gradient(rgba(255,255,255,0), rgba(255,255,255,1));
filter: progid:DXImageTransform.Microsoft.gradient(GradientType=0,startColorstr='#00FFFFFF', endColorstr='#FFFFFFFF');
background-repeat: no-repeat;
}
#commentsTabView .authorRow>div:not(.contactsmenu-popover) {
display: inline-block;
vertical-align: middle;
}
#commentsTabView .authorRow>div.hidden {
#commentsTabView .hidden {
display: none !important;
}
/** set min-height as 44px to ensure that it fits the button sizes. **/
#commentsTabView .comment .authorRow {
min-height: 44px;
}
#commentsTabView .comment .authorRow .tooltip {
/** because of the padding on the element, the tooltip appear too far up,
adding this brings them closer to the element**/
margin-top: 5px;
}
#commentsTabView .comments li .message .avatar-name-wrapper,
.atwho-view-ul * .avatar-name-wrapper,
#commentsTabView .comment .authorRow {
@ -164,47 +159,48 @@
.atwho-view-ul * .avatar-name-wrapper {
white-space: nowrap;
}
#commentsTabView .comment .author,
#commentsTabView .comment .date {
opacity: .5;
}
#commentsTabView .comment .author {
max-width: 210px;
text-overflow: ellipsis;
overflow: hidden;
white-space: nowrap;
}
#commentsTabView .comment .date {
margin-left: auto;
/** this is to fix the tooltip being too close due to the margin-top applied
to bring the tooltip closer for the action icons **/
padding: 10px 0px;
}
#commentsTabView .comments li .message {
#commentsTabView .comments > li:not(.newCommentRow) .message {
padding-left: 40px;
display: inline-flex;
flex-wrap: wrap;
align-items: center;
}
#commentsTabView .comment .action {
opacity: 0;
padding: 5px;
}
#commentsTabView .comment:hover .action {
opacity: 0.3;
padding: 14px;
display: block;
}
#commentsTabView .comment .action:hover {
#commentsTabView .comment .action:hover,
#commentsTabView .comment .action:focus {
opacity: 1;
}
#commentsTabView .comment .action.delete,
#commentsTabView .comment .deleteLoading {
position: absolute;
right: 0;
#commentsTabView .newCommentRow .action-container {
margin-left: auto;
}
#commentsTabView .comment.disabled {
#commentsTabView .comment.disabled .message {
opacity: 0.3;
}
#commentsTabView .comment.disabled .action {
visibility: hidden;
display: none;
}
#commentsTabView .message.error {
@ -215,4 +211,4 @@
.app-files .action-comment {
padding: 16px 14px;
}
}

View file

@ -0,0 +1,124 @@
/*
* Copyright (c) 2018
*
* This file is licensed under the Affero General Public License version 3
* or later.
*
* See the COPYING-README file.
*
*/
/* global Handlebars */
(function() {
var TEMPLATE_MENU =
'<ul>' +
'{{#each items}}' +
'<li>' +
'<a href="#" class="menuitem action {{name}} permanent" data-action="{{name}}">' +
'{{#if iconClass}}' +
'<span class="icon {{iconClass}}"></span>' +
'{{else}}' +
'<span class="no-icon"></span>' +
'{{/if}}' +
'<span>{{displayName}}</span>' +
'</li>' +
'{{/each}}' +
'</ul>';
/**
* Construct a new CommentsModifyMenuinstance
* @constructs CommentsModifyMenu
* @memberof OC.Comments
*/
var CommentsModifyMenu = OC.Backbone.View.extend({
tagName: 'div',
className: 'commentsModifyMenu popovermenu bubble menu',
_scopes: [
{
name: 'edit',
displayName: t('comments', 'Edit comment'),
iconClass: 'icon-rename'
},
{
name: 'delete',
displayName: t('comments', 'Delete comment'),
iconClass: 'icon-delete'
}
],
initialize: function() {
},
events: {
'click a.action': '_onClickAction'
},
template: Handlebars.compile(TEMPLATE_MENU),
/**
* Event handler whenever an action has been clicked within the menu
*
* @param {Object} event event object
*/
_onClickAction: function(event) {
var $target = $(event.currentTarget);
if (!$target.hasClass('menuitem')) {
$target = $target.closest('.menuitem');
}
OC.hideMenus();
this.trigger('select:menu-item-clicked', event, $target.data('action'));
},
/**
* Renders the menu with the currently set items
*/
render: function() {
this.$el.html(this.template({
items: this._scopes
}));
},
/**
* Displays the menu
*/
show: function(context) {
this._context = context;
for(var i in this._scopes) {
this._scopes[i].active = false;
}
var $el = $(context.target);
var offsetIcon = $el.offset();
var offsetContainer = $el.closest('.authorRow').offset();
// adding some extra top offset to push the menu below the button.
var position = {
top: offsetIcon.top - offsetContainer.top + 48,
left: '',
right: ''
};
position.left = offsetIcon.left - offsetContainer.left;
if (position.left > 200) {
// we need to position the menu to the right.
position.left = '';
position.right = this.$el.closest('.comment').find('.date').width();
this.$el.removeClass('menu-left').addClass('menu-right');
} else {
this.$el.removeClass('menu-right').addClass('menu-left');
}
this.$el.css(position);
this.render();
this.$el.removeClass('hidden');
OC.showMenu(null, this.$el);
}
});
OCA.Comments = OCA.Comments || {};
OCA.Comments.CommentsModifyMenu = CommentsModifyMenu;
})(OC, OCA);

View file

@ -21,24 +21,22 @@
'<div class="loading hidden" style="height: 50px"></div>';
var EDIT_COMMENT_TEMPLATE =
'<div class="newCommentRow comment" data-id="{{id}}">' +
'<{{tag}} class="newCommentRow comment" data-id="{{id}}">' +
' <div class="authorRow">' +
' <div class="avatar currentUser" data-username="{{actorId}}"></div>' +
' <div class="author currentUser">{{actorDisplayName}}</div>' +
'{{#if isEditMode}}' +
' <a href="#" class="action delete icon icon-delete has-tooltip" title="{{deleteTooltip}}"></a>' +
' <div class="deleteLoading icon-loading-small hidden"></div>'+
' <div class="action-container">' +
' <a href="#" class="action cancel icon icon-close has-tooltip" title="{{cancelText}}"></a>' +
' </div>' +
'{{/if}}' +
' </div>' +
' <form class="newCommentForm">' +
' <div contentEditable="true" class="message" data-placeholder="{{newMessagePlaceholder}}">{{message}}</div>' +
' <input class="submit icon-confirm" type="submit" value="" />' +
'{{#if isEditMode}}' +
' <input class="cancel pull-right" type="button" value="{{cancelText}}" />' +
'{{/if}}' +
' <input class="submit icon-confirm has-tooltip" type="submit" value="" title="{{submitText}}"/>' +
' <div class="submitLoading icon-loading-small hidden"></div>'+
' </form>' +
'</div>';
'</{{tag}}>';
var COMMENT_TEMPLATE =
'<li class="comment{{#if isUnread}} unread{{/if}}{{#if isLong}} collapsed{{/if}}" data-id="{{id}}">' +
@ -46,7 +44,8 @@
' <div class="avatar{{#if isUserAuthor}} currentUser{{/if}}" {{#if actorId}}data-username="{{actorId}}"{{/if}}> </div>' +
' <div class="author{{#if isUserAuthor}} currentUser{{/if}}">{{actorDisplayName}}</div>' +
'{{#if isUserAuthor}}' +
' <a href="#" class="action edit icon icon-rename has-tooltip" title="{{editTooltip}}"></a>' +
' <a href="#" class="action more icon icon-more has-tooltip"></a>' +
' <div class="deleteLoading icon-loading-small hidden"></div>' +
'{{/if}}' +
' <div class="date has-tooltip live-relative-timestamp" data-timestamp="{{timestamp}}" title="{{altDate}}">{{date}}</div>' +
' </div>' +
@ -64,12 +63,11 @@
id: 'commentsTabView',
className: 'tab commentsTabView',
_autoCompleteData: undefined,
_commentsModifyMenu: undefined,
events: {
'submit .newCommentForm': '_onSubmitComment',
'click .showMore': '_onClickShowMore',
'click .action.edit': '_onClickEditComment',
'click .action.delete': '_onClickDeleteComment',
'click .cancel': '_onClickCloseComment',
'click .comment': '_onClickComment',
'keyup div.message': '_onTextChange',
@ -114,9 +112,9 @@
actorId: currentUser.uid,
actorDisplayName: currentUser.displayName,
newMessagePlaceholder: t('comments', 'New comment …'),
deleteTooltip: t('comments', 'Delete comment'),
submitText: t('comments', 'Post'),
cancelText: t('comments', 'Cancel')
cancelText: t('comments', 'Cancel'),
tag: 'li'
}, params));
},
@ -166,7 +164,7 @@
emptyResultLabel: t('comments', 'No comments yet, start the conversation!'),
moreLabel: t('comments', 'More comments …')
}));
this.$el.find('.comments').before(this.editCommentTemplate({}));
this.$el.find('.comments').before(this.editCommentTemplate({ tag: 'div'}));
this.$el.find('.has-tooltip').tooltip();
this.$container = this.$el.find('ul.comments');
this.$el.find('.avatar').avatar(OC.getCurrentUser().uid, 32);
@ -401,6 +399,24 @@
// it is the case when writing a comment and mentioning a person
$message = $el;
}
if (!editionMode) {
var self = this;
// add the dropdown menu to display the edit and delete option
var modifyCommentMenu = new OCA.Comments.CommentsModifyMenu();
$el.find('.authorRow').append(modifyCommentMenu.$el);
$el.find('.more').on('click', _.bind(modifyCommentMenu.show, modifyCommentMenu));
self.listenTo(modifyCommentMenu, 'select:menu-item-clicked', function(ev, action) {
if (action === 'edit') {
self._onClickEditComment(ev);
} else if (action === 'delete') {
self._onClickDeleteComment(ev);
}
});
}
this._postRenderMessage($message, editionMode);
},
@ -567,15 +583,13 @@
var $comment = $(ev.target).closest('.comment');
var commentId = $comment.data('id');
var $loading = $comment.find('.deleteLoading');
var $commentField = $comment.find('.message');
var $submit = $comment.find('.submit');
var $cancel = $comment.find('.cancel');
var $moreIcon = $comment.find('.more');
$commentField.prop('contenteditable', false);
$submit.prop('disabled', true);
$cancel.prop('disabled', true);
$comment.addClass('disabled');
$loading.removeClass('hidden');
$moreIcon.addClass('hidden');
$comment.data('commentEl', $comment);
this.collection.get(commentId).destroy({
success: function() {
@ -584,10 +598,8 @@
},
error: function() {
$loading.addClass('hidden');
$moreIcon.removeClass('hidden');
$comment.removeClass('disabled');
$commentField.prop('contenteditable', true);
$submit.prop('disabled', false);
$cancel.prop('disabled', false);
OC.Notification.showTemporary(t('comments', 'Error occurred while retrieving comment with ID {id}', {id: commentId}));
}
@ -668,6 +680,12 @@
}, {
success: function(model) {
self._onSubmitSuccess(model, $form);
if(model.get('message').trim() === model.previous('message').trim()) {
// model change event doesn't trigger, manually remove the row.
var $row = $form.closest('.comment');
$row.data('commentEl').removeClass('hidden');
$row.remove();
}
},
error: function() {
self._onSubmitError($form, commentId);

View file

@ -4,6 +4,7 @@
"commentcollection.js",
"commentsummarymodel.js",
"commentstabview.js",
"commentsmodifymenu.js",
"filesplugin.js",
"activitytabviewplugin.js",
"vendor/Caret.js/dist/jquery.caret.min.js",

View file

@ -190,7 +190,7 @@ describe('OCA.Comments.CommentsTabView tests', function() {
expect(fetchStub.notCalled).toEqual(true);
view.$el.find('.showMore').click();
view.$el.find('.showMore').trigger('click');
expect(fetchStub.calledOnce).toEqual(true);
});
@ -398,10 +398,10 @@ describe('OCA.Comments.CommentsTabView tests', function() {
$message = $newCommentForm.find('.message');
$submitButton = $newCommentForm.find('.submit');
});
afterEach(function() {
tooltipStub.restore();
afterEach(function() {
tooltipStub.restore();
});
it('does not displays tooltip when limit is far away', function() {
$message.val(createMessageWithLength(3));
$message.trigger('change');
@ -490,18 +490,21 @@ describe('OCA.Comments.CommentsTabView tests', function() {
it('shows edit link for owner comments', function() {
var $comment = view.$el.find('.comment[data-id=1]');
expect($comment.length).toEqual(1);
$comment.find('.action.more').trigger('click');
expect($comment.find('.action.edit').length).toEqual(1);
});
it('does not show edit link for other user\'s comments', function() {
var $comment = view.$el.find('.comment[data-id=2]');
expect($comment.length).toEqual(1);
$comment.find('.action.more').trigger('click');
expect($comment.find('.action.edit').length).toEqual(0);
});
it('shows edit form when clicking edit', function() {
var $comment = view.$el.find('.comment[data-id=1]');
$comment.find('.action.edit').click();
$comment.find('.action.more').trigger('click');
$comment.find('.action.edit').trigger('click');
expect($comment.hasClass('hidden')).toEqual(true);
var $formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
@ -510,7 +513,8 @@ describe('OCA.Comments.CommentsTabView tests', function() {
it('saves message and updates comment item when clicking save', function() {
var $comment = view.$el.find('.comment[data-id=1]');
$comment.find('.action.edit').click();
$comment.find('.action.more').trigger('click');
$comment.find('.action.edit').trigger('click');
var $formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
expect($formRow.length).toEqual(1);
@ -544,7 +548,8 @@ describe('OCA.Comments.CommentsTabView tests', function() {
it('saves message and updates comment item with mentions when clicking save', function() {
var $comment = view.$el.find('.comment[data-id=3]');
$comment.find('.action.edit').click();
$comment.find('.action.more').trigger('click');
$comment.find('.action.edit').trigger('click');
var $formRow = view.$el.find('.newCommentRow.comment[data-id=3]');
expect($formRow.length).toEqual(1);
@ -591,13 +596,14 @@ describe('OCA.Comments.CommentsTabView tests', function() {
it('restores original comment when cancelling', function() {
var $comment = view.$el.find('.comment[data-id=1]');
$comment.find('.action.edit').click();
$comment.find('.action.more').trigger('click');
$comment.find('.action.edit').trigger('click');
var $formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
expect($formRow.length).toEqual(1);
$formRow.find('textarea').val('modified\nmessage');
$formRow.find('.cancel').click();
$formRow.find('.cancel').trigger('click');
expect(saveStub.notCalled).toEqual(true);
@ -614,12 +620,8 @@ describe('OCA.Comments.CommentsTabView tests', function() {
it('destroys model when clicking delete', function() {
var destroyStub = sinon.stub(OCA.Comments.CommentModel.prototype, 'destroy');
var $comment = view.$el.find('.comment[data-id=1]');
$comment.find('.action.edit').click();
var $formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
expect($formRow.length).toEqual(1);
$formRow.find('.delete').click();
$comment.find('.action.more').trigger('click');
$comment.find('.action.delete').trigger('click');
expect(destroyStub.calledOnce).toEqual(true);
expect(destroyStub.thisValues[0].id).toEqual(1);
@ -630,15 +632,11 @@ describe('OCA.Comments.CommentsTabView tests', function() {
$comment = view.$el.find('.comment[data-id=1]');
expect($comment.length).toEqual(0);
// form row is gone
$formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
expect($formRow.length).toEqual(0);
destroyStub.restore();
});
it('does not submit comment if the field is empty', function() {
var $comment = view.$el.find('.comment[data-id=1]');
$comment.find('.action.edit').click();
$comment.find('.action.edit').trigger('click');
$comment.find('.message').val(' ');
$comment.find('form').submit();
@ -646,7 +644,7 @@ describe('OCA.Comments.CommentsTabView tests', function() {
});
it('does not submit comment if the field length is too large', function() {
var $comment = view.$el.find('.comment[data-id=1]');
$comment.find('.action.edit').click();
$comment.find('.action.edit').trigger('click');
$comment.find('.message').val(createMessageWithLength(view._commentMaxLength * 2));
$comment.find('form').submit();
@ -659,7 +657,7 @@ describe('OCA.Comments.CommentsTabView tests', function() {
beforeEach(function() {
updateMarkerStub = sinon.stub(OCA.Comments.CommentCollection.prototype, 'updateReadMarker');
});
afterEach(function() {
afterEach(function() {
updateMarkerStub.restore();
});

View file

@ -93,6 +93,7 @@ module.exports = function(config) {
'apps/comments/js/commentmodel.js',
'apps/comments/js/commentcollection.js',
'apps/comments/js/commentsummarymodel.js',
'apps/comments/js/commentsmodifymenu.js',
'apps/comments/js/commentstabview.js',
'apps/comments/js/filesplugin.js'
],
@ -223,7 +224,7 @@ module.exports = function(config) {
// serve images to avoid warnings
files.push({pattern: 'core/img/**/*', watched: false, included: false, served: true});
files.push({pattern: 'core/css/images/*', watched: false, included: false, served: true});
// include core CSS
files.push({pattern: 'core/css/*.css', watched: true, included: true, served: true});
files.push({pattern: 'tests/css/*.css', watched: true, included: true, served: true});