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

Add textarea support #379

Merged
merged 1 commit into from
Jul 5, 2016
Merged

Add textarea support #379

merged 1 commit into from
Jul 5, 2016

Conversation

maxpoulin64
Copy link
Member

@maxpoulin64 maxpoulin64 commented Jun 5, 2016

Attemps to do the exact same thing as #330, but in a way I find is a lot more reliable and way less overengineered.

  • Supports pressing Shift+Enter to add a new line
  • Doesn't break history
  • Automatically grows
  • Handles multiline on the server-side (doing it on the client works, but doing it on the server makes it bulletproof for when we add plugins and other things. Otherwise extra lines are passed raw, and while it's okay because we have /quote someone might disable those and turn it into a flaw)
  • Keeps scroll position in chat
  • Doesn't grow for a single line, keeping the current behavior (which I find less distracting when typing long messages)
  • Better positionning of the textarea

Slight note: I'm wondering if when sending multiline messages, we should automatically assume it's a multiline message and not attempt to parse commands at all. Thoughts?

@maxpoulin64 maxpoulin64 added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jun 5, 2016
@maxpoulin64 maxpoulin64 added this to the 2.0.0 milestone Jun 5, 2016
@maxpoulin64 maxpoulin64 force-pushed the textarea branch 6 times, most recently from 9fe68c7 to b81fef9 Compare June 5, 2016 03:54
@xPaw
Copy link
Member

xPaw commented Jun 5, 2016

Just tested, this is nice! 👍

Also nice use of flexbox.

EDIT: Would it be possible to keep nick/send buttons at the bottom when the textarea grows?

@maxpoulin64
Copy link
Member Author

EDIT: Would it be possible to keep nick/send buttons at the bottom when the textarea grows?

Sure, done. Also fixed themes, I had forgotten about them >.>

@dgw
Copy link
Contributor

dgw commented Jun 5, 2016

Slight note: I'm wondering if when sending multiline messages, we should automatically assume it's a multiline message and not attempt to parse commands at all. Thoughts?

I'd vote for parsing each line for commands still. I do an awful lot of multiline stuff on desktop with commands.

@xPaw
Copy link
Member

xPaw commented Jun 5, 2016

I'd vote for parsing each line for commands still.

That's my opinion too. If we were to somehow parse commands differently and support multiline arguments, we would only introduce odd bugs. The current solution is fine.

@AlMcKinlay AlMcKinlay mentioned this pull request Jun 6, 2016
3 tasks
@xPaw xPaw self-assigned this Jun 8, 2016
@astorije
Copy link
Member

FYI, I'm planning to review this ASAP :-)

@AlMcKinlay
Copy link
Member

@astorije You still wanting to be second reviewer? Not trying to rush you, just would like it in reasonably soon :-) if you aren't bothered, I could probably take the second review in the next few days. But I don't want to steal it from you if you are planning to do it at the weekend or whatever.

@astorije astorije self-assigned this Jun 30, 2016
@astorije
Copy link
Member

@YaManicKill, feel free to review if you want, but I really would like to give it a stab whenever I get the chance to :-) I'm hoping end of this week or early next week.

@williamboman
Copy link
Member

Nice! Noticed two things right off the bat;

@astorije
Copy link
Member

astorije commented Jul 2, 2016

So, I finally got to play a little bit with this. I also checked out #330 and ran it on the side to test when behavior seemed off or simply by curiosity of how differently it would act.

I noticed the following things that bugged me when I tested this PR:

  • Text expands to the right as you type instead of triggering a new line. While this is consistent with what we currently have, it has been driving me nuts since my very first day on Shout :-)
    Now that we switch to a multiline textarea, we should not do this anymore. Add multiple lines to input. #330 goes to the next line when reaching the right end, actually.
  • Scrolling using the touchpad doesn't doesn't work, it tries to scroll the entire window (at least on Mac OS X, latest Chrome). It works OK on Add multiple lines to input. #330.
  • I haven't tested with other themes, but on default theme text lines are too condensed. We should have same or similar spacing between 2 lines in the textarea than between 2 lines (of the same message) in the chat window. It's subtle, but it's noticeable, and I'm thinking it might also be tricky on mobile.
  • Nick stays at the bottom of the textarea when it grows. On Add multiple lines to input. #330, it stays at the top, which I prefer, but it's no big deal, we can improve that later.
  • On desktop (haven't tested on mobile), one must type 14 lines before a scrollbar appears. I think that's too much. 5 or 8 maybe (6.5 on Add multiple lines to input. #330)? Also (but I'm nitpicking), max height should display entire lines and not half ones.

That's a lot of comments for a first test, but clearly the most annoying points are the first two and I'd be happy to give a +1 when these 2 are fixed. All others can be addressed in later PR(s).
I noticed the first 2 points behave OK on #330, but note that I'm talking about behavior and UX only. I haven't looked at the code of neither of these PRs yet, it's just from a usage perspective. @maxpoulin64, @YaManicKill, what do you think, are these first 2 points easy to fix here or should we go back to #330 instead? I really know nothing of the benefits of one over the other aside from the points listed here.

Thanks for this, @maxpoulin64 (and @YaManicKill), I can't wait to have this in, it's gonna be a great enhancement!!

@maxpoulin64
Copy link
Member Author

Text expands to the right as you type instead of triggering a new line. While this is consistent with what we currently have, it has been driving me nuts since my very first day on Shout :-)
Now that we switch to a multiline textarea, we should not do this anymore. #330 goes to the next line when reaching the right end, actually.

Yes, this have been done on purpose. I prefer this behavior myself as it's more consistent with what every other IRC client do, and it reduces the impression that the user can type in as much as they want. I can change it if you prefer, it's just a CSS property (which I can put it back the way I like in custom CSS afterwards).

I originally changed it that way because multiline text on mobile just feels odd and takes most of the remaining screen estate. So I opted for the compact way.

Scrolling using the touchpad doesn't doesn't work, it tries to scroll the entire window (at least on Mac OS X, latest Chrome). It works OK on #330.

I have noticed that too, but assumed that was just the way textareas worked. I'll investigate further, as I definitely noticed that on mobile. I just assumed it was because it was horizontal scrolling, haven't tried vertical.

I haven't tested with other themes, but on default theme text lines are too condensed. We should have same or similar spacing between 2 lines in the textarea than between 2 lines (of the same message) in the chat window. It's subtle, but it's noticeable, and I'm thinking it might also be tricky on mobile.

That's the default, but thankfully an easy change line-height: 120%;. Will do.

Nick stays at the bottom of the textarea when it grows. On #330, it stays at the top, which I prefer, but it's no big deal, we can improve that later.

That was requested by @xPaw above, and I agree with him on this one (was previously vertically centered). Not obvious right away on desktop, but it quickly does on mobile because that means the send arrow is right up the keyboard. Having both down is nice for consistency.

On desktop (haven't tested on mobile), one must type 14 lines before a scrollbar appears. I think that's too much. 5 or 8 maybe (6.5 on #330)? Also (but I'm nitpicking), max height should display entire lines and not half ones.

Right now I have it limited to a third of the total screen height, so it's variable. I can limit it to a predefined height, but a set line count is a bit harder as we don't have access to individual lines but only the element's height. It's already pretty tricky to make that bastard size properly ;)

@astorije
Copy link
Member

astorije commented Jul 2, 2016

I prefer this behavior myself as it's more consistent with what every other IRC client do, and it reduces the impression that the user can type in as much as they want. I can change it if you prefer, it's just a CSS property (which I can put it back the way I like in custom CSS afterwards).

Oh, yes please! ❤️
To be fair, IRC clients are not usually known for their UX/UI, and this was a strong argument in favor of Shout at the time. On the other hand, I don't know any other messaging client supporting auto-expanding text inputs that expand to the right on top of expanding vertically.

@AlMcKinlay
Copy link
Member

Oh, yes please! ❤️
To be fair, IRC clients are not usually known for their UX/UI, and this was a strong argument in favor of Shout at the time. On the other hand, I don't know any other messaging client supporting auto-expanding text inputs that expand to the right on top of expanding vertically.

Agreed

@jjardon
Copy link

jjardon commented Jul 2, 2016

Hi,

as a data point; we use IRC for our team communications, and we need to be able to send arbitrary amount of messages when we do our stand ups. Actually, this is the only remaining feature to recommend the use of thelounge

@maxpoulin64 maxpoulin64 force-pushed the textarea branch 2 times, most recently from 0ce39de to 4e9acc9 Compare July 4, 2016 23:42
@maxpoulin64
Copy link
Member Author

@astorije: Just updated. Now always multiline, supports touch scrolling (was a silly bug). Line height has been set to 1.5, should be good enough. Should also fix some sligh alignment issues, I'm now using the flexbox to align the textarea instead of relying on the padding (since scrolling apparently doesn't account for the padding for some reason).

@astorije
Copy link
Member

astorije commented Jul 5, 2016

Amazing job, @maxpoulin64! I know a lot of folks will be happy about this :-)
👍 and merging!

@astorije astorije merged commit 114295a into thelounge:master Jul 5, 2016
@maxpoulin64 maxpoulin64 deleted the textarea branch July 5, 2016 05:04
@maxpoulin64
Copy link
Member Author

🎆 🎉

@dgw
Copy link
Contributor

dgw commented Jul 5, 2016

🎇🎆🎉🎆🎉🎆🎇

AlMcKinlay added a commit that referenced this pull request Jul 6, 2016
Fix slight bugs introduced by #379 and #465
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@xPaw xPaw unassigned astorije and xPaw Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants