Skip to content

Add special color case handling for older titles - #1

Open
CodyNinja1 wants to merge 4 commits into
openplanet-nl:mainfrom
CodyNinja1:main
Open

Add special color case handling for older titles#1
CodyNinja1 wants to merge 4 commits into
openplanet-nl:mainfrom
CodyNinja1:main

Conversation

@CodyNinja1

@CodyNinja1 CodyNinja1 commented Sep 4, 2026

Copy link
Copy Markdown

This PR adds a new option for Parser.toHTML/parse called allowShortColors: boolean, that controls short-color behaviour to allow compatibility between older titles and this library, alongside a new testcase for said option called should handle incomplete color codes by cutting text off.

Below is an example of it:

import Parser from './Parser.js';

// retains current behaviour, color is padded with zeros
// "$fhello, world" == "$f00hello, world"
Parser.toHTML("$fhello, world", { allowShortColors: false });

// non-hex characters are replaced with zeros, and 2 chars are "eaten" from the string.
// the tag "$fhe" is treated as a color, and is equivilant to "$f0e"
// "$fhello, world" == "$f0ello, world"
Parser.toHTML("$fhello, world", { allowShortColors: true });

@codecat codecat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR! Damn, I really typo'd DefaultOptions everywhere huh..

I haven't looked at the actual implementation yet (I suspect there might be a better way?) but here's some preliminary notes.

Comment thread tests/index.js Outdated
Parser.toHTML('$fRed'),
'<span style="color: #ff0000;">Red</span>');

// this type of interpretation is unique to games older than maniaplanet, where "$fhe" is the color, and non-hex characters are treated as zeros

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should aim to have descriptions like these as part of the DefaultOptions file instead.

Comment thread src/Parser.js Outdated
nextToken.text += c;
}

shortColor = false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Missing ;

@codecat

codecat commented Sep 4, 2026

Copy link
Copy Markdown
Member

I also think the name of the setting is misleading and doesn't really explain its intention. I have a better idea for the long run, but we should probably rename this to something like consumeNonHexColors? (Maybe there's something shorter that's still concise)

@CodyNinja1

CodyNinja1 commented Sep 4, 2026

Copy link
Copy Markdown
Author

Is interpretNonHexAsColor a good name?
Also, should I move the comment to DefaultOptions.js or not right now?

@CodyNinja1
CodyNinja1 requested a review from codecat September 4, 2026 10:05
@codecat

codecat commented Sep 4, 2026

Copy link
Copy Markdown
Member

That name works. Yeah, you can add the documentation for that option in DefaultOptions. I can fix the formatting and add more documentation later 👍

@CodyNinja1

Copy link
Copy Markdown
Author

Should be good to go now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants