Bring back multiline comments #1407

rullzer · 2016-09-14T12:24:14Z

Fixes #942

Before:

After:

Which gives us multiline comments again! Whoohoo.
Only thing that we need to think about is how to handle the scroll bar (move to left?)

CC: @nickvergessen @nextcloud/designers @MorrisJobke

mention-bot · 2016-09-14T12:24:17Z

@rullzer, thanks for your PR! By analyzing the annotation information on this pull request, we identified @MorrisJobke, @nickvergessen and @jancborchardt to be potential reviewers

eppfel · 2016-09-14T13:15:28Z

Scrollbar on the left would be super weird.
Why is the button inside the text field anyway? With single line comments, you were able to send via return, so maybe the arrow signals this behavior. But for multiline the action should be separate. (to show some status inside the field seems right)

eppfel · 2016-09-14T13:16:08Z

I mean, the cancel button is there, too.

nickvergessen · 2016-09-14T13:53:19Z

core/js/core.json

@@ -11,7 +11,8 @@
 		"backbone/backbone.js",
 		"es6-promise/dist/es6-promise.js",
 		"davclient.js/lib/client.js",
-		"clipboard/dist/clipboard.js"
+		"clipboard/dist/clipboard.js",
+        "autosize/dist/autosize.min.js"


tabs vs spaces

rullzer · 2016-09-14T17:27:31Z

@eppfel well I was following #942 (comment)

But feel free to hijack to improve the design :)

jancborchardt · 2016-09-21T23:43:26Z

@rullzer @eppfel this field should be autosized, meaning there should never ever be a scrollbar. :) Roeland, it seems you already added it?

rullzer · 2016-10-03T13:44:18Z

Okidoki now fully autosizing

Review time!

MorrisJobke · 2016-10-04T07:44:24Z

PhantomJS 2.1.1 (Linux 0.0.0) OCA.Comments.CommentsTabView tests editing comments saves message and updates comment item when clicking save FAILED
    Expected Object({ message: 'New message' }) to equal Object({ message: 'modified message' }).
    /drone/src/github.com/nextcloud/server/apps/comments/tests/js/commentstabviewSpec.js:335:45

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer · 2016-10-04T11:08:07Z

Fixed

nickvergessen · 2016-10-04T11:14:39Z

Works 👍

nickvergessen · 2016-10-04T11:16:26Z

What I just noticed, when you have leading or trailing newlines/whitespace they are displayed in the comment, until you do a page refresh.
But I guess that is not related and should be fixed in a second PR?

rullzer · 2016-10-04T11:21:00Z

Yeah lets fix that separatly.

MorrisJobke · 2016-10-05T15:22:49Z

I will review this - there are only some layout glitches between firefox and Chrome.

MorrisJobke · 2016-10-05T15:33:41Z

apps/comments/css/comments.css

@@ -25,7 +25,7 @@

 #commentsTabView .newCommentForm .submit {
 	position: absolute;
-	top: 1px;
+	bottom: 1px;


With this it looks in Firefox good, but in Chrome not that good. Then I could change this to 5px to look in Chrome good, but in Firefox not centered. The reason is that the surrounding element (a <form>) is in Chrome not fitted around the textarrea but a bit longer at the bottom.

@nextcloud/designers Any idea how we could solve this?

This is only 75 px in height in Firefox (and those are the 4 additional pixels that are needed in Chrome) 😞

This is how it looks without the change to bottom: 5px in chrome:

Maybe @skjnldsv can help with this maybe? 😉

I failed too, for some reason the form height is calculated differently in FF/Chrome and therefore the absolute positioning looks different.

However, now that we want to bring back a dedicated login button in #1641 as the inline one caused confusion and here it looks also weird as soon as the comment is expanded to multiple lines, I thought about changing this form to use a dedicated button too. cc @jancborchardt

@ChristophWurst @MorrisJobke You need to put a display:block on the textarea. :)
It should do the trick and the form should now take the full textarea size.

firefox
chrome

Espina2 · 2016-10-11T09:47:13Z

@ChristophWurst @jancborchardt @nextcloud/designers

What you guys think in making something like this. (Very quick mockup)
Its very similar to What I did for the news page.

rullzer · 2016-10-11T10:38:03Z

@Espina2 o wow! I like it!
But lets first get this in and then change it :D

skjnldsv

👍

MorrisJobke · 2016-10-11T10:38:48Z

But lets first get this in and then change it :D

Yep makes sense 👍 We should separate the different topics here.

skjnldsv · 2016-10-11T10:40:33Z

Always forgot the approval doesn't work on reviews! 😑

LGTM 👍

MorrisJobke · 2016-10-11T10:43:19Z

Tested and works 👍

jancborchardt · 2016-10-11T12:09:18Z

@Espina2 very nice! Just two things:

We don’t need the extra cointainer around the comment, whitespace is enough.
Right now we sort comments with the newest up top, and the form is also on the top. This we should keep especially when we switch to introducing a timeline (combining activity and comments)
In general, the introduction of a timeline necessitates that the layout of the comments and activites is in line with each other: 📑 Sidebar: combine file Activity and Comments into »Activity« timeline tab (possibly also "Versions") #658 – maybe best work on a mockup for that and we can continue discussion there :) (cause the mockup from me there is already quite old and only crude)

Espina2 · 2016-10-11T13:33:22Z

@jancborchardt Thanks dude.

Yup, we should discuss this in your issue. This is only a quick sketch for showing how can work visually not to much thinking in it. 👍

rullzer added enhancement design Design, UI, UX, etc. 2. developing Work in progress labels Sep 14, 2016

rullzer added this to the Nextcloud 11.0 milestone Sep 14, 2016

nickvergessen reviewed Sep 14, 2016
View reviewed changes

rullzer force-pushed the multiline_comments branch from a074da9 to 04b8afc Compare September 14, 2016 17:42

This was referenced Sep 20, 2016

Small UX improvements nextcloud/calendar#72

Closed

UI confusion between login and alternative login (registration for instance) #1404

Closed

rullzer force-pushed the multiline_comments branch from 04b8afc to 54613ab Compare October 3, 2016 13:43

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 3, 2016

rullzer added 3 commits October 4, 2016 13:04

Add autosize

c5d24a6

Add autosize to comments input

5ca50d9

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix js test

c83b3e6

rullzer force-pushed the multiline_comments branch from 54613ab to c83b3e6 Compare October 4, 2016 11:07

MorrisJobke self-assigned this Oct 5, 2016

MorrisJobke reviewed Oct 5, 2016

View reviewed changes

nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 6, 2016

MorrisJobke removed their assignment Oct 6, 2016

MorrisJobke added the help wanted label Oct 6, 2016

ChristophWurst self-assigned this Oct 10, 2016

fix textarea submit

dcddd4b

skjnldsv force-pushed the multiline_comments branch from 0ce6f17 to dcddd4b Compare October 11, 2016 10:33

MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 11, 2016

skjnldsv approved these changes Oct 11, 2016

View reviewed changes

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews help wanted labels Oct 11, 2016

MorrisJobke merged commit 675230f into master Oct 11, 2016

MorrisJobke deleted the multiline_comments branch October 11, 2016 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back multiline comments #1407

Bring back multiline comments #1407

rullzer commented Sep 14, 2016

mention-bot commented Sep 14, 2016

eppfel commented Sep 14, 2016

eppfel commented Sep 14, 2016 •

edited

Loading

nickvergessen Sep 14, 2016

rullzer commented Sep 14, 2016

jancborchardt commented Sep 21, 2016

rullzer commented Oct 3, 2016

MorrisJobke commented Oct 4, 2016

rullzer commented Oct 4, 2016

nickvergessen commented Oct 4, 2016

nickvergessen commented Oct 4, 2016

rullzer commented Oct 4, 2016

MorrisJobke commented Oct 5, 2016

MorrisJobke Oct 5, 2016

MorrisJobke Oct 5, 2016

MorrisJobke Oct 5, 2016

ChristophWurst Oct 11, 2016

skjnldsv Oct 11, 2016

skjnldsv Oct 11, 2016

Espina2 commented Oct 11, 2016 •

edited

Loading

rullzer commented Oct 11, 2016

skjnldsv left a comment

MorrisJobke commented Oct 11, 2016

skjnldsv commented Oct 11, 2016

MorrisJobke commented Oct 11, 2016

jancborchardt commented Oct 11, 2016 •

edited

Loading

Espina2 commented Oct 11, 2016 •

edited

Loading

Bring back multiline comments #1407

Bring back multiline comments #1407

Conversation

rullzer commented Sep 14, 2016

mention-bot commented Sep 14, 2016

eppfel commented Sep 14, 2016

eppfel commented Sep 14, 2016 • edited Loading

nickvergessen Sep 14, 2016

Choose a reason for hiding this comment

rullzer commented Sep 14, 2016

jancborchardt commented Sep 21, 2016

rullzer commented Oct 3, 2016

MorrisJobke commented Oct 4, 2016

rullzer commented Oct 4, 2016

nickvergessen commented Oct 4, 2016

nickvergessen commented Oct 4, 2016

rullzer commented Oct 4, 2016

MorrisJobke commented Oct 5, 2016

MorrisJobke Oct 5, 2016

Choose a reason for hiding this comment

MorrisJobke Oct 5, 2016

Choose a reason for hiding this comment

MorrisJobke Oct 5, 2016

Choose a reason for hiding this comment

ChristophWurst Oct 11, 2016

Choose a reason for hiding this comment

skjnldsv Oct 11, 2016

Choose a reason for hiding this comment

skjnldsv Oct 11, 2016

Choose a reason for hiding this comment

Espina2 commented Oct 11, 2016 • edited Loading

rullzer commented Oct 11, 2016

skjnldsv left a comment

Choose a reason for hiding this comment

MorrisJobke commented Oct 11, 2016

skjnldsv commented Oct 11, 2016

MorrisJobke commented Oct 11, 2016

jancborchardt commented Oct 11, 2016 • edited Loading

Espina2 commented Oct 11, 2016 • edited Loading

eppfel commented Sep 14, 2016 •

edited

Loading

Espina2 commented Oct 11, 2016 •

edited

Loading

jancborchardt commented Oct 11, 2016 •

edited

Loading

Espina2 commented Oct 11, 2016 •

edited

Loading