Chat input scrollback/history with additional fixes #189

Closed
wants to merge 3 commits into
from

Projects

None yet

2 participants

@SpenserJ
Contributor

Added input scrollback via up and down arrow keys - Closes #121
Escape key clears the message input box
Combining sendMessage code between pressing enter and clicking send
Let the onchange event of #chat-input enable/disable the send button
Only send a message if there is a message to send.
Previously, you could click the send button with an empty
message input, and a blank line would appear in the chat window.

@SpenserJ SpenserJ Various improvements to the chat.js view
Added input scrollback via up and down arrow keys
Escape key clears the message input box
Combining sendMessage code between pressing enter and clicking send
Let the onchange event of #chat-input enable/disable the send button
Only send a message if there is a message to send.
    Previously, you could click the send button with an empty
    message input, and a blank line would appear in the chat window.
8a8e7fb
@thedjpetersen thedjpetersen and 1 other commented on an outdated diff Dec 23, 2012
assets/js/views/chat.js
@@ -66,24 +82,35 @@ var ChatView = Backbone.View.extend({
event.preventDefault();
}
keydownEnter = (event.keyCode === 13);
+ var activeChat = irc.chatWindows.getActive();
+ if (typeof activeChat !== 'undefined' && (event.keyCode == 38 || event.keyCode == 40)) {
+ var direction = (event.keyCode == 38) ? -1 : 1,
+ input_scrollback = activeChat.get('input_scrollback'),
+ new_position = input_scrollback.index + direction;
@thedjpetersen
thedjpetersen Dec 23, 2012 Owner

Please declare new_position first i.e. var new_position

@SpenserJ
SpenserJ Dec 23, 2012 Contributor

That is actually a multi-line var declaration, hence the indentation level. It would be equivalent to

var direction = (event.keyCode == 38) ? -1 : 1;
var input_scrollback = activeChat.get('input_scrollback'),
var new_position = input_scrollback.index + direction;
@thedjpetersen
thedjpetersen Dec 23, 2012 Owner

good point I didn't noticed the ,

@thedjpetersen thedjpetersen commented on an outdated diff Dec 23, 2012
assets/js/views/chat.js
@@ -66,24 +82,35 @@ var ChatView = Backbone.View.extend({
event.preventDefault();
}
keydownEnter = (event.keyCode === 13);
+ var activeChat = irc.chatWindows.getActive();
+ if (typeof activeChat !== 'undefined' && (event.keyCode == 38 || event.keyCode == 40)) {
+ var direction = (event.keyCode == 38) ? -1 : 1,
+ input_scrollback = activeChat.get('input_scrollback'),
+ new_position = input_scrollback.index + direction;
+ if (input_scrollback.index == null) { // Are we at the end of the list?
+ if (direction == 1) return; // We can't scroll past the end of the list
@thedjpetersen
thedjpetersen Dec 23, 2012 Owner

please use === and on the line above

@SpenserJ
Contributor

David, I'm a bit rusty on the capabilities of the pull request system, but if I remember correctly, you should be able to merge SpenserJ/subway@600123b manually after you merge this request. If not, I can send a fresh pull request with both.

@SpenserJ
Contributor

Scratch that, seems I can push to a specific branch and it adds to the pull request, so you should have both in the same now. Let me know if you want me to tweak anything else in it.

@thedjpetersen
Owner

Right you beat me to the punch - I'll go ahead and review again.

@thedjpetersen thedjpetersen and 1 other commented on an outdated diff Dec 24, 2012
assets/js/views/chat.js
irc.socket.emit('say', {target: irc.chatWindows.getActive().get('name'), message:message});
}
$('#chat-input').val('');
- });
+ $('#chat-button').addClass('disabled');
@thedjpetersen
thedjpetersen Dec 24, 2012 Owner

Is there a place that you remove this class?

@SpenserJ
SpenserJ Dec 24, 2012 Contributor

At the end of every keyUp we trigger $('#chat-input').change(), which enables or disables the input based on whether there is actually content. Although I just noticed that this should call $('#chat-input').change() as well, so that so that all of the enable/disable code is in a single location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment