Skip to content
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

Submit comments with Enter and use Shift+Enter for new lines #7252

Merged
merged 4 commits into from
Dec 8, 2017

Conversation

danxuliu
Copy link
Member

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@codecov
Copy link

codecov bot commented Nov 23, 2017

Codecov Report

Merging #7252 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #7252      +/-   ##
============================================
+ Coverage     50.93%   50.93%   +<.01%     
  Complexity    24710    24710              
============================================
  Files          1586     1586              
  Lines         94144    94145       +1     
  Branches       1364     1364              
============================================
+ Hits          47948    47956       +8     
+ Misses        46196    46189       -7
Impacted Files Coverage Δ Complexity Δ
apps/comments/js/commentstabview.js 81.98% <100%> (+1.92%) 0 <0> (ø) ⬇️
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.53% <0%> (+0.24%) 136% <0%> (ø) ⬇️
core/search/js/search.js 38% <0%> (+0.45%) 0% <0%> (ø) ⬇️

@nickvergessen
Copy link
Member

I still think we shouldn't do this, because on comments people should write longer, not more often...

@blizzz
Copy link
Member

blizzz commented Nov 23, 2017

I still think we shouldn't do this, because on comments people should write longer, not more often...

Seconded, I am not convinced of the change.

@danxuliu
Copy link
Member Author

Seconded, I am not convinced of the change.

Me neither; I am just granting @jancborchardt a wish :-P

@jancborchardt
Copy link
Member

As commented on the other issue in Talk:

The issue atm is more that not really a lot of people know we have Comment functionality, so we need to make it as dead-simple to use as possible.

Also, we should have this consistent with the Talk app.

@jancborchardt
Copy link
Member

(That is also, every time I used comments myself or had it tested by other people, when they used enter they were confused as to why it didn't submit. That's also why I asked for the change in Talk too.
Saying "Comments should be longer" is wishful thinking but not how people actually use it.)

@nickvergessen
Copy link
Member

nickvergessen commented Nov 27, 2017

It used to be a two lines textarea, but then our designer wanted a self-growing one-line div-element. A single line suggests enter to be send. So yes, currently it does not match your expectations, because you changed one part but not the other. I still think we should just make the input-div a bit bigger then it is clear that enter will create new lines.

It's the same on facebook. If you post your own item (big box with multiple lines height by default), enter means new line. On their chatty 1-line comments input, enter is submit.

@jancborchardt
Copy link
Member

@danxuliu @MorrisJobke what about the failures? I'd lile to merge this. :)

@danxuliu
Copy link
Member Author

what about the failures?

Basically it is complaining that no unit tests were added. And it is right :-O

I'd lile to merge this. :)

Give me a second to add the tests ;-)

When finding ".message" elements on "view.$el" the message area for the
new comment form and all the comments were matched. Now the selector was
restricted to match only the message area for the new comment form.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Copy link
Member Author

That was a long second... :-P

Anyway I could not add proper unit tests for entering a new line instead of posting a comment when shift+enter was used, as apparently the exact behaviour in that case depends on the browser. PhantomJS does not seem to handle key events on content editable divs, so the test only verifies that the default behaviour would have been executed when typing shift+enter. Better than nothing...

@danxuliu
Copy link
Member Author

danxuliu commented Dec 7, 2017

I have noticed that pressing Enter when the autocomplete popover for mentions was shown caused the comment to be submitted instead of the mention to be completed; I have fixed that in the last commit.

When the autocomplete popover is shown the At.js plugin listens on the
message input field for key down events, and when Enter is pressed it
adds the selected item to the message. However, as "_onTypeComment" also
handles key down events for the message input field, when Enter was
pressed the comment was submitted and At.js had no chance to add the
item before that happened. Now when Enter is pressed and the
autocomplete popover is shown the comment is not submitted, and thus
At.js adds the selected item to the message as expected.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Copy link
Member Author

danxuliu commented Dec 7, 2017

I have amended the last commit to include unit tests too for pressing Enter when the autocomplete popover for mentions is shown (it was trickier than expected... but it is done now ;-) ).

@MorrisJobke
Copy link
Member

All CI failures are unrelated.

@MorrisJobke MorrisJobke merged commit 5724e75 into master Dec 8, 2017