Skip to content
This repository has been archived by the owner on Oct 13, 2021. It is now read-only.

True color support #96

Merged
merged 3 commits into from
Jan 4, 2018
Merged

True color support #96

merged 3 commits into from
Jan 4, 2018

Conversation

d3nw0
Copy link
Contributor

@d3nw0 d3nw0 commented Jan 2, 2018

My old pull request contained bad code and failed tests. This is a better version of it that will not break old code.

@d3nw0 d3nw0 mentioned this pull request Jan 2, 2018
@d3nw0 d3nw0 changed the title New: Added true color support Added true color support Jan 2, 2018
@d3nw0 d3nw0 changed the title Added true color support True color support Jan 2, 2018
@cceckman
Copy link
Contributor

cceckman commented Jan 3, 2018

Suggestion (which could go in a followup PR): Rather than having a new type and a switch statement,

type Color tcell.Color
const (
  ColorYellow Color = tcell.ColorYellow
  ...
)

(and corresponding other changes).

That means that you don't have to import tcell to get the standard ~8 colors if you already have tui around, but removes a translation layer. It's also a little clearer as to where to find more information.

@marcusolsson
Copy link
Owner

The switch is a remnant from when tui supported both termbox and tcell. Now that we're only supporting tcell, I think it's safe to rewrite it like @cceckman hinted. I'd like to avoid users having to import tcell at all, and would rather mirror the color set from tcell. Type aliases might be more convenient here(?) but that would require >=go1.8.

@marcusolsson marcusolsson merged commit 51685d3 into marcusolsson:master Jan 4, 2018
@d3nw0
Copy link
Contributor Author

d3nw0 commented Jan 5, 2018

I would suggest removing the color constant variables, and using tui.GetColor(name string) Color, which has more color coverage.

@cceckman
Copy link
Contributor

cceckman commented Jan 5, 2018

Devil's advocate: Strings are not type-safe, enums are. tui.GetColor("fuschia") vs. tui.GetColor("fucshia") is a runtime error, while tui.ColorFuschia vs. tui.ColorFucshia is a compile-time error.

It would be useful to have a string-based variant - for cases where the color is a user input - but I that's not always.

@d3nw0
Copy link
Contributor Author

d3nw0 commented Jan 5, 2018

Yes, but in-case the string is misspelled the default color will be returned. That way a runtime error will not exist.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants