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

ensure Todo highlight on dark bg is at least 233 #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atweiden
Copy link

@atweiden atweiden commented Jul 18, 2020

  • note
    • existing s:dark_bg_2 handling is beneficial for hl-VertSplit
      • hence
        • this variable is kept in tact
  • previously
    • with g:seoul256_background set to 233
      • todo syntax was rendered with dark background contrast
        • as bg colors dropped to #000000
  • now
    • use the equivalent to light bg handling for dark bg todos
      • for equivalently balanced visual effect
      • see: s:light_bg_2

@junegunn
Copy link
Owner

junegunn commented Jan 5, 2023

with g:seoul256_background set to 233

  • todo syntax was rendered with visually jarring effect

    • as bg colors dropped to #000000

Do you find it really bad? I think it looks okay.

image

@atweiden
Copy link
Author

atweiden commented Jan 5, 2023

Reason for PR:

" s:light_bg_2 becomes background colour for |hl-Todo| when colorscheme
" is seoul256-light
"
" n.b. it cannot exceed 256, hence with |g:seoul256_light_background|
" set to 256, |hl-Todo| background colour matches seoul256-light
" background colour, thereby enabling opting out of background colour
" contrast
let s:light_bg_2 = min([s:light_bg + 2, 256])

" s:dark_bg_2 becomes background colour for |hl-Todo| when colorscheme
" is seoul256
"
" n.b. if |g:seoul256_background| is set to 233, |hl-Todo| background
" colour drops to 16 — this is noticeably higher contrast than in the
" seoul256-light corollary, where users can completely opt out of contrast
let s:dark_bg_2 = s:dark_bg > 233 ? s:dark_bg - 2 : 16

This PR should only affect users who set g:seoul256_background to 233.

I agree it’s better said to be a minor nitpick.

@junegunn
Copy link
Owner

junegunn commented Jan 5, 2023

thereby enabling opting out of background colour contrast

So the difference is only noticeable to the users who switch between light and dark modes, using 256 for the light background color, and 233 for the dark background. Anyone not using the exact combo will not notice the difference.

What I didn't understand at first was your initial expression visually jarring effect, because #000000 isn't much darker than 232, and such slight contrast is consistently present across higher g:seoul256_background values, except for 256.

256 is the only exception here, because 256 is the lightest color, and it's technically impossible to get a lighter background, but 233 is not #000000, so thankfully we can pick a darker color. This PR is suggesting that we should also make 233 an exception (no contrast). Maybe a better approach would be to support 16?

@atweiden
Copy link
Author

atweiden commented Jan 6, 2023

So the difference is only noticeable to the users who switch between light and dark modes, using 256 for the light background color, and 233 for the dark background. Anyone not using the exact combo will not notice the difference.

Yes. Alas, I use seoul256-light for gvim and seoul256 for vim.

Maybe a better approach would be to support 16?

Support for 16 would be a welcome addition. 233 is perfectly fine, but either way.

- note
  - existing s:dark_bg_2 handling is beneficial for hl-VertSplit
    - hence
      - this variable is kept in tact
- previously
  - with g:seoul256_background set to 233
    - todo syntax was rendered with dark background contrast
      - as bg colors dropped to #000000
- now
  - use the equivalent to light bg handling for dark bg todos
    - for equivalently balanced visual effect
    - see: s:light_bg_2
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