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

Save our eyes; colours must have contrast by default #315

Closed
haf opened this issue Apr 4, 2019 · 11 comments
Closed

Save our eyes; colours must have contrast by default #315

haf opened this issue Apr 4, 2019 · 11 comments
Labels

Comments

@haf
Copy link
Owner

haf commented Apr 4, 2019

E.g. red on black has horrible contrast. https://www.viget.com/articles/color-contrast/

These are the de-facto colours now:

image

@haf haf added the bug label Apr 4, 2019
haf added a commit that referenced this issue Apr 4, 2019
@haf haf changed the title 256 colours by default Save our eyes; colours must have contrast by default Apr 4, 2019
@AnthonyLloyd AnthonyLloyd removed the bug label Apr 5, 2019
@AnthonyLloyd
Copy link
Contributor

AnthonyLloyd commented Apr 5, 2019

My reasoning for making 8 the default was that it was the middle ground between no color and 256. It's supported almost everywhere (not teamcity etc). Also 256 doesn't work in Travis, AppVeyor, VSCode and ConEmu (my terminal, it doesn't look good as currently set up). Not strongly fixed to this default though.

8 colours isn't a bug, we need to support it. Most people use a colour scheme to make red look ok.

It's more complicated than a right or wrong choice unfortunately. The 256 colour don't look correct in ConEmu in any scheme I've found, plus peoples scheme have to support other things that are 8 colours like fake etc.

Luckily you cover 256 and I cover 8 so we can at least make both look good where they tend to be used.

Default is tricky. There's an argument for making it 0 and people having to make and active choice for 8 vs 256 based on what they use.

@jzabroski
Copy link

It's important not to do an automatic color contrast algorithm. They don't work. It's best to pick a color palette that works.

@haf haf added the bug label Apr 8, 2019
@haf
Copy link
Owner Author

haf commented Apr 8, 2019

I consider it a bug that I can't read the text, like you saw in the screenshot. This is my own fault; I chose the red colour (I presume?) for 256 (you haven't changed it?)

  • isatty is the standard, we should use it (if the API exists) and use 0 colours if it's not (and the API exists). We should assume our users have modern terminals by default, like all modern libraries do (mocha, rspec, cypress, ruby gems and so on).
  • For Windows we should discuss; but we could default to 0 if you think that is appropriate. But: you can install the ansicolor.dll https://stackoverflow.com/questions/16755142/how-to-make-win32-console-recognize-ansi-vt100-escape-sequences on Windows, which is what I assumed everyone did.
    • In lieu of having ansicolor installed, how about we default to 8 colours in on Windows, 256 otherwise, and let windows users opt-in to 256 or opt-out with 0?
  • This issue should track having a better colour palette. I suggest we make the red background, with white text instead of red text.
    • that way it'll look much better with 8 colours and make it more obvious what is part of the "-"-diff text, too.
  • @jzabroski I completely agree; my screenshots of the colour analysis was based on the actual observed colours on my laptop.

@AnthonyLloyd
Copy link
Contributor

I've not changed the 256 colours.

isatty is available as a PInvoke on Linux but not Windows as far as I know.

I don't know the modern libraries you list but fake, dotnet seem to use 8 colours.

I've never heard of ansicolor.dll.

We could go for 8 on Windows and 256 on Linux. Checking isatty if available and defaulting to zero in that case. This will mean Travis will still be incorrect as it doesn't support it.

@haf
Copy link
Owner Author

haf commented Apr 8, 2019

This will mean Travis will still be incorrect as it doesn't support it.

No, I don't think it will, since Travis doesn't say it's a TTY.

We could go for 8 on Windows and 256 on Linux. Checking isatty if available and defaulting to zero in that case.

Yes, this seems to be the Ultimate™ tradeoff! 🦋

@AnthonyLloyd
Copy link
Contributor

AnthonyLloyd commented Apr 8, 2019

No, I don't think it will, since Travis doesn't say it's a TTY.

But it supports 8 colours? Makes the story more complicated. May be best to just leave isatty out.

8 windows, 256 non-windows may at least flush out some more views.

@haf
Copy link
Owner Author

haf commented Apr 8, 2019

You're right; it seems it supports 8 colours and it reports to be a TTY to programs. It also seems to parse away cursor-move chars.

No, it's not best to leave isatty out IMO; it's like a guard; if it returns false, let's use no colours or fall back to the stuff I've written below. (For (another) reference: https://aweirdimagination.net/2015/02/21/256-color-terminals/)

This https://github.com/dtolnay/isatty/blob/master/src/lib.rs — suggests using getconsolemode on Windows; and the docs suggest checking the flag ENABLE_PROCESSED_OUTPUT = 0x0001

However, this is new; if the terminal supports 256 colours, you have this set:

image

So how about this?

let isatty fd =
  if Env.windows then Native.getConsoleMode() &&& 0x0001 > 0
  else Native.isatty()

let colours =
  if isatty stdout && (env "TERM").Contains "256color" then Colours 256
  elif isatty stdout then Colours 8
  else Colours 0

(Also Windows is a ghetto; why are you using it? 🧟‍♀️)

@AnthonyLloyd
Copy link
Contributor

I guess my problem with getconsolemode is similar to isatty. I bet it indicates 256 but 256 looks washed out in ConEmu. It's really down to the colour scheme. In the end the docs are going to have to say if it looks rubbish try switching your colour scheme or switch the --colours.

(Also Windows is a ghetto; why are you using it? 🧟‍♀️)

Need to support windows stuff like WPF, Excel. Macs are expensive (relative) and too lock in for me. Plus I prefer Windows 10 UI. Easy Linux/Docker is tempting but would be a smell if really necessary.

@haf
Copy link
Owner Author

haf commented Jul 2, 2019

It's really down to the colour scheme.

We can do a good basic colour scheme in this lib and then it's up to every person how the terminal is configured to look.

I've implemented the IsWindows check for now, but I think my suggested code above is actually the best way to move forward with the colours-problem. It might also be an idea to remove the mutable static colours field and move to using DVars, so that if Expecto noticies itself being used inside the .net core test running (for example), it can disable progress bars and the like. What do you think?

@AnthonyLloyd
Copy link
Contributor

This isn't the normal default colour. It's just if you use Expect without the run functions.

@haf
Copy link
Owner Author

haf commented Jul 2, 2019

Yes I know; like in .fsx scripts.

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

No branches or pull requests

3 participants