tinted-theming / base16-emacs

Base16 themes for Emacs
MIT License
380 stars 76 forks source link

Shm overrides (Fix for some dark themes) #47

Closed dstechenko closed 7 years ago

dstechenko commented 7 years ago

Well, I decided to double check some of the themes after the merge #46. And turns out quarantine-face does not play nice with some of the dark ones.

What would be great is to have a warning/error color with some opacity reduced. But sadly we don't have such, so I propose to re-use highlight color.

Also use ':inherit' instead of a concrete color.

A dark theme quarantine-fac before:

screen shot 2017-05-04 at 6 38 45 am

A dark theme quarantine-face after:

screen shot 2017-05-04 at 6 39 21 am

A light theme quarantine-face after (before attached in the previous PR #46):

screen shot 2017-05-04 at 6 37 57 am

What do you think?

belak commented 7 years ago

If I read the information correctly the quarantine-face was meant for syntax errors, so making it a tiny bit more noticable might be best. Other error faces have added a wavy underline, but I'm not sure if you want to go that route.

I'd like to reserve :inherit for faces which are definitely directly related so (and please correct me if you think they are directly related) I think it may not be the best fit here.

Also, while you're in the area, could you change the heading shm to structured-haskell-mode and rearrange the section?

Thanks for taking another look at this!

dstechenko commented 7 years ago

The current-face as I see it should be related to the selected region, hence that :inherit there. It shows the selected section in a way. I'd like to mark quarantine-face with something that would show that's an error, but sadly only a few colors work good as a background. Like we could re-use highlight, but if you try using any foreground color as a background - you start getting weird color combos in some of the themes.

Pretty much we only have base00(default), base01(highlight), base02(region) and base07(unused light background) to be safely used. base07 - works bad with some of the themes. base00 - does not show anything base02 - should be used for the selected section base01 - the only background that is left

What would be cool if we could take the error color and make it dim a lil' (using shades of default) and then use it to color the background. But that's not something that aligns with base16 guidelines.

So how should we approach this? What do you think?

belak commented 7 years ago

Fair enough. If that's the case, :inherit may make sense for current-face. Did you see the flycheck faces? Would something like that work?

dstechenko commented 7 years ago

Okay, it looks really good. Thanks! I will use the same styling.

dstechenko commented 7 years ago

One more thing to consider. I'm thinking regarding this :inherit region part. We may want to switch to :inherit highlight instead.

Due to the fact that one could try selecting some text to copy and it will overlap with the current-face. So I think it would be a better fit to use highlight instead.

What do you think?

UPD. I checked it, but seems like shm puts current-face over the selected region, so it's OK to use the same face anyways. Guess we are good then.

dstechenko commented 7 years ago

Again, I found they use :foreground color to mark the whole quarantined block. Example

Which one is better, what do you think?

With :foreground

screen shot 2017-05-04 at 8 37 56 pm

With :underline

screen shot 2017-05-04 at 8 38 23 pm

I'm pretty much OK with both of them. I think the underline is a lil' bit better, so pushing it. But you decide, tho. :)

belak commented 7 years ago

I prefer the underline, as it's consistent with our other error types. Thanks for your contribution!