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

Implement color hotkeys #810

Merged
merged 2 commits into from
Mar 31, 2017
Merged

Implement color hotkeys #810

merged 2 commits into from
Mar 31, 2017

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Dec 17, 2016

Same key bindings as http://www.ircbeginner.com/ircinfo/colors.html

Even though the characters are invisible (in Chrome at least), it's still useful as an initial implementation.

I moved all hotkey bindings into their own scope (add ?w=1 to url for saner diff)

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Dec 17, 2016
contextMenuContainer.hide();
});
var colorsHotkeys = {
k: "\x03",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command+k is already used by clearing the screen on macOS (see L1298)... I don't want us to clearing this to command+l because it focuses the URL bar, which is immensely useful.
Any other hot key we can use for colors?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason not to simply bind Ctrl+K on every platform? That's what a number of existing clients use, and it would reduce The Lounge's learning curve if that keybinding remained the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@astorije On windows it uses ctrl+shift+l. While ctrl+l focuses address bar, shift modifier does not. Can we do the same on mac?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So cmd+shift+L to clear the screen and cmd+K to set up color? Works for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

astorije
astorije previously approved these changes Dec 20, 2016
Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on this! 👏

@DanielOaks
Copy link
Contributor

Nice, this works well 👍. After this gets merged perhaps making it easier to see that you've pressed this would be good -- right now it's just a little hard to tell that you've actually entered one of these chars until you hit enter.

@astorije
Copy link
Member

Sadly this is almost impossible with the current textarea, but if we ever go for a rich-content equivalent, it would. Not on the agenda for now though.
I reckon @xPaw had a crazy idea with tailor-made font or something, but I can't find it in the issues/PRs.

@xPaw
Copy link
Member Author

xPaw commented Dec 20, 2016 via email

@astorije
Copy link
Member

Ah ah, I see the result, pretty crazy indeed! I don't think the font-family falls back on the next font when a char is missing (i.e. all non-control-code chars in the case of a font with just B, I, U symbols), does it?

@astorije astorije added this to the 2.3.0 milestone Jan 4, 2017
}
});
Mousetrap.bind([
"command+shift+l",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write a changelog entry for this change.

"ctrl+" + hotkey
], function(e) {
e.preventDefault();
input.val(input.val() + colorsHotkeys[e.key]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not care about cursor position, need to fix.

@xPaw
Copy link
Member Author

xPaw commented Mar 12, 2017

I've updated the PR to correctly insert formatting characters at the position where the cursor is. Also supports selections.

@astorije astorije self-requested a review March 13, 2017 05:59
@astorije
Copy link
Member

@xPaw, I added the new shortcuts in the help windows, let me know what you think. I'll review your recent changes as well.

@xPaw
Copy link
Member Author

xPaw commented Mar 18, 2017

@astorije I don't quite understand why we have separate sections for windows and macOS bindings. The only difference is the meta key (ctrl vs ⌘)

@astorije
Copy link
Member

Because that might not be the case for long, due to the different nature of OSes. As a matter of fact, they used to be different before a recent PR.
Also, I think we should quickly hide this distinction. It must not be difficult to detect if we are on Windows/Linux or macOS on the front-end and display the according section only. I am less certain on how to be sure we have a keyboard on the client or not.

Copy link
Member

@AlMcKinlay AlMcKinlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@xPaw xPaw merged commit 212703d into master Mar 31, 2017
@xPaw xPaw deleted the xpaw/color-hotkeys branch March 31, 2017 16:17
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@AlMcKinlay AlMcKinlay removed their assignment 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.

5 participants