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 support for 0x04 hex colors #1100

Merged
merged 1 commit into from
May 6, 2017
Merged

Add support for 0x04 hex colors #1100

merged 1 commit into from
May 6, 2017

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Apr 28, 2017

Ref: https://modern.ircdocs.horse/formatting.html#hex-color

At least IRCCloud and AdiIRC support it.

cc @Bonuspunkt

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Apr 28, 2017
@@ -12,6 +13,9 @@ const UNDERLINE = "\x1f";
// integers, `XX` is the text color and `YY` is an optional background color.
const colorRx = /^(\d{1,2})(?:,(\d{1,2}))?/;

// 6-char Hex color code matcher
const hexColorRx = /^([a-fA-F0-6]{6})?/;
Copy link
Contributor

Choose a reason for hiding this comment

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

pattern seems wrong, should be /^([0-9a-f]{6})?/i ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that 0-6 is wrong.

@@ -104,12 +109,70 @@ describe("parseStyle", () => {
expect(actual).to.deep.equal(expected);
});

it("should parse hex colors", () => {
const input = "test \x04FFFFFFnice \x02\x04RES006 \x0303,04\x04ff00FFcolored";
Copy link
Contributor

@Bonuspunkt Bonuspunkt Apr 28, 2017

Choose a reason for hiding this comment

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

is hex formatting foreground only? (if you have access to the clients, do you mind to verify?)
also more color variation would be nice (see issue with pattern)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, only foreground.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, AdiIRC seems to allow foreground and background. I should make it more clear on the doc that it allows setting both foreground and background colours with hex in this way

Copy link
Member

Choose a reason for hiding this comment

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

@xPaw, do you want/need to update your PR to account for foreground/background as per @DanielOaks' comment or not?

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 currently does not support background colors. Considering @DanielOaks didn't know that too, I wonder how many clients implement it, compared to just the foreground color.

Copy link
Member

Choose a reason for hiding this comment

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

Right, it doesn't. But don't we want to be the first IRC client leading the way forward? :D (Ok ok it's a minor lead 😅)

Copy link
Member Author

Choose a reason for hiding this comment

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

Considering there is no formal definition, we want to know how it's already implemented in other clients.

Copy link
Member

@astorije astorije Apr 28, 2017

Choose a reason for hiding this comment

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

Fair enough.

@xPaw
Copy link
Member Author

xPaw commented Apr 29, 2017

Added support for background colors.

@xPaw xPaw added this to the 2.3.0 milestone Apr 29, 2017
@xPaw xPaw merged commit 7ae364e into master May 6, 2017
@xPaw xPaw deleted the xpaw/0x04 branch May 6, 2017 10:43
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