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

A more colourful menu #236

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thazhemadam
Copy link

This PR adds colours to the menu options and also makes the colours for options that can be toggled (except for debuginfo) more expressive.

Before:

before

After:

after

@vchuravy vchuravy requested a review from timholy October 6, 2021 19:24
@vchuravy vchuravy linked an issue Oct 6, 2021 that may be closed by this pull request
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Personally I do like the new look! One concern is red/green colorblind users; it's the most common form, apparently about 1/12 of all men suffer from colorblindness https://aapos.org/glossary/color-blindness. Any chance we can choose a more colorblind-friendly set of colors?

src/ui.jl Outdated Show resolved Hide resolved
@thazhemadam
Copy link
Author

thazhemadam commented Oct 6, 2021

I went with the red-green combo because it felt intuitive - green means yes and red means no; but I didn't take color blindness into account!

Any chance we can choose a more colorblind-friendly set of colors?

Absolutely! Do you have any in mind or a personal preference from these (or any other) palettes?

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #236 (6471471) into master (6880b9e) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
+ Coverage   82.85%   82.89%   +0.03%     
==========================================
  Files           7        7              
  Lines        1021     1023       +2     
==========================================
+ Hits          846      848       +2     
  Misses        175      175              
Impacted Files Coverage Δ
src/ui.jl 83.87% <100.00%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6880b9e...6471471. Read the comment docs.

@timholy
Copy link
Member

timholy commented Oct 7, 2021

I agree completely that for the non-colorblind red/green is indeed very intuitive, and that no other color combo quite matches it. But so many people are affected by some form of colorblindness that to me I think we have to account for their needs.

I am definitely not an expert and would defer to your or others' choices, but to me both the green/purple (lower left in "accessible palettes) or the blue/magenta (lower right) seem reasonably intuitive.

Since red-green color blindness is common, use a blue-magenta combo
instead. Also, make the toggled option bold and change the color of
first line of the menu to green to reduce the amount of blue in the
menu.
@timholy
Copy link
Member

timholy commented Oct 11, 2021

Hmm, I wonder if we need to customize the color choice dependent on system? On my Ubuntu desktop, the purple kind of fades into the background for obvious reasons:

image

@thazhemadam
Copy link
Author

thazhemadam commented Oct 11, 2021

Hmm, I wonder if we need to customize the color choice dependent on system?

I don't think that would be a good idea as there's a good chance that any colour combination we pick might appear faded in a similar manner, depending upon users' terminal settings. I don't think there's one size that fits all for this, so we should probably just pick one colour palette and stick with it.

On a related note, I also think brighter colours might make the content more easily discernible (the yellow-pink palette here seems to be one of the brighter pairs, but I'm not sure they're the most intuitive 😅 ).

@timholy
Copy link
Member

timholy commented Oct 11, 2021

What about making it configurable? E.g.,

startup.jl

const CTHULHU_ON = :green
const CTHULHU_OFF = :red

for those of us who can see those colors (and default to the ones you've chosen). Internally you can use isdefined(Main, :CTHULHU_ON) ? Main.CTHULHU_ON : :blue etc. Alternatively you could look in ENV.

@thazhemadam
Copy link
Author

What about making it configurable?

I like this idea. But what do you think about making it configurable through Cthulhu.CONFIG instead?

@anandijain
Copy link

i'd love this feature! to be friendly to colorblind, maybe we could default the coloring to be as-is (off) and leave the config to another PR?

@thazhemadam thanks for doing this

@thazhemadam thazhemadam force-pushed the at/pretty-menus branch 4 times, most recently from ef2c459 to ee4e0c1 Compare March 15, 2023 04:13
@fingolfin
Copy link
Contributor

It's a pity this went stale :-(.

I agree we should take the needs of colorblind users in mind, but: my understanding is that someone who is e.g. red/green blind may not be able to differentiate between certain red and green colors (if they have the same brightness etc.), but they can still see them.

So wouldn't that mean for such a user, the effect would be that they don't see different colors -- so in effect exactly what we have no. I.e there would be no regression for them, just no improvement.

So while of course we should ultimately allow them to benefit, too (e.g. by making the colors configurable -- perhaps via Preferences.jl), wouldn't it still make sense to merge this improvement now, and add the color preferences later?

@simeonschaub simeonschaub added the enhancement New feature or request label Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colourize headings for menu options
6 participants