Improve activity bar styling and introduce color library

[musicfuel]

Purpose

This change is to improve the visual appearance and usability of the activity bar when applying a color.

Additionally, adds tinycolor to:

• perform color manipulation
• improve flexibility of color input (see Accepted String Input)
• enhance color capabilities while reducing extension code base

What

The default styling of the activity bar in VS Code presents the activity bar in a slightly different shade than the status and title bars to better visually delineate it. It also presents the icons in a more subdued shade when not active or not as the inactive state.

Currently, Peacock colorizes the activity bar as the same overall background color as the others. It also sets the inactive foreground color of the icons to the same as the active foreground color. Both of these combine to degrade the look and usability of the activity bar when using a Peacock color.

This change uses a combination of color modifications to properly stylize the activity bar to better match the default behavior of VS Code so that the switch to Peacock is a bit more seamless.

How to Test

Tests have not been written specific to the new styling, but will be added. All current tests pass following refactoring. To test manually, use Peacock to set random colors or particularly light or dark colors and verify that icons remain visible.

What to Check

The library allowed for refactoring of the existing color-library code to reduce it. It also manages color input with a much larger array of input that allows for most anything that CSS understands. Existing functionality should be checked, especially the use of named colors since the library replaces some of that functionality.

[musicfuel]

@johnpapa I intended to start this with a proposal, but was driving so quickly playing with it that it made more since to continue on and submit as a request. The feature itself is small, but the underlying work is a bit more extensive. I see the new contributing guidelines regarding this and will adhere to them going forward.

[johnpapa]

Thank you for the thoughtful PR! Interesting idea. I'm wondering if it is better to have the activity not match the title and status bars. I see that it's a slightly different and lighter shade. My thoughts:

0. is it desirable for most users?
1. if that is desirable for users, then should it be a setting to enable or disable it?
2. should this not be code but instead just set a setting , which already exists? (set activity bar to lighter shade manually)

I think I lean to #1 above. also, is tinycolor adding some value? less dependencies are often desirable

definitely need changelog, readme, and tests.

anyone else have thoughts?

[musicfuel]

I agree with you on the dependency being less desirable and I wasn't sure about it, but it made sense to me to take color calculation out of the primary code base in this case and the library enables several more input methods. This enables the extension to focus more on setting management rather than details like hex and rgb channel calculations. I've worked a lot in color things like this and would be happy to remove the dependency to inline more of the necessary behavior if you prefer.

Personally for me it is desirable because I am used to the default dark theme out of the box in code and when I first used peacock I was a little thrown off with the solid color around 3/4 of the window. It is a minor thing for the visual aspect, but I really think the inactive state of the activity bar icons is the much more important element here from a usability perspective.

I agree with number 1 as well and had thought about it as a setting. I can look to that as I work on the other elements.

[johnpapa]

Thanks for the thoughtful reply

That helps. I think you’re right that tinycolor might be a good choice here. I just like to make sure I’m careful before taking on extra deps. Will be interesting to see how big the end result it. Easy to find out.

Let’s go with option 1.

We’d need that plus tests that make sure nothing regresses. Just because so much logic was altered.

Do you feel you can take this on? If so I’ll wait to review.

Really appreciate your contribution and willingness to discuss this. Thanks!

[musicfuel]

Sounds good. I can definitely take it on and will have updates for review soon.

[johnpapa]

Awesome!

I released v0.5.0 which has some breaking changes. I documented it best I could based on the proposal in the PR from #47

just a heds up.

[musicfuel]

Thanks for the heads up! That rebase proved interesting, but I was able to merge everything together without too much trouble.

I was able to make some great progress on this and have this capability supported in preferences for any of the affected elements. I added quite few tests for my new features and updates proposed changes to the README and CHANGELOG. Please take a look at the code and give it a review when you have time.

One thing to note is that I started down my peacock.elementAdjustments as a { [string]: string} structure based on the previous peacock.affectedElements setting when it was an array of strings. Now that it is spread out, I am not sure it feels right and have two ideas to spread it out like the others if you think it worthwhile:

• Create peacock.adjustActivityBar, peacock.adjustStatusBar, and peacock.adjustTitleBar properties as enums of "darken", "lighten", "none". This would present them as simple dropdown in the Settings editor which is nice, but it might get confused with the others a bit.
• Combine the boolean affected properties and the enum adjustment properties into a single property for each element that is an enum of "none", "color", "color darken", "color lighter". The idea here is that "none" is like false, "color" is like true, and either of the others are true with an adjustment.

I played with each of those ideas a bit, but left my settings as is for now since I'm looking for feedback.

[johnpapa]

Thanks. This is great! I hear you on the properties. I think having 1 property that combines both is fine if we make it super clear in the property and value wording.

peacock.colorizeActivityBar: {
  "type": "string",
  "enum": [
    "no effect",
    "enabled",
    "enabled - lighten",
    "enabled - darken"
  ]

The problem I see is I don't know what would be clear here. What do you think of the above?

If we go with 2 properties for each (6 total) it is a bit clearer what each does, but it would be odd to set the one to "darken" and the boolean to false, and have no effect. I mean, someone might be confused by that.

This is tough.

[johnpapa]

I'm going to merge this as-is just to get it into master.

Let's continue this discussion and see if we want to make the adjustments (pun intended) soon.

[johnpapa]

Also - should we get rid of light and dark foreground now?

[johnpapa]

Thinking out loud - if we combine these, everything is in a list which is set via the settings.json. Keeping them separate means the checkbox is in the settings UI.

[musicfuel]

You've been busy and I am just catching up on this. Thanks for the extra commits and the merge!

I like what you proposed...

peacock.colorizeActivityBar: {
  "type": "string",
  "enum": [
    "no effect",
    "enabled",
    "enabled - lighten",
    "enabled - darken"
  ]

...and your discussion on why it is tough is the exact mental gymnastic exercise I went through. I think combining them into one enum where the "no effect" is false and one of the "enabled" is true works, but again it is tough. It feels like we add a lot of complexity.

What do you mean about "should we get rid of light and dark foreground now?" Do you mean just the ability to configure the colors for them in settings?

[johnpapa]

Great points. I think we release what we have and gather feedback. Adjust if we need to. Ok?

Regarding light and dark foreground - those are separate settings now that get applied based on whether the background is deemed contrasting with it. I had made those a specific setting in case someone wanted to over them. It looks like tinycolor maight make that obsolete. Can you take a look at clarify that?

[musicfuel]

Sounds good on gathering feedback.

Regarding the light and dark foreground, those settings are still respected. The getForegroundColorHex function uses tinycolor to determine the relative brightness of the color and then uses either the light or dark foreground from settings as a basis for the color. I just tested locally and I am able to manage the color from settings, sometimes to my detriment (a pure red #ff0000 foreground on Angular red is particularly bad).

So, the settings are indeed still in effect, but should they be? We could remove them and use the defaults and/or tinycolor to determine one as an option. I'm not sure how much value being able to directly configured them adds to the extension.

[johnpapa]

It seems it could be more harmful to use them. If tinycolor is solving the problem already.

I’d be happy to reduce and simplify.

[johnpapa]

Here is where i introduced it ... the more i think about this, the less i think we need it and I like having less settings

https://github.com/johnpapa/vscode-peacock/blob/master/CHANGELOG.md#011

[musicfuel]

I agree. I'd be happy to work on this aspect. To better follow the typical practice shall I create an issue around this and then connect the PR to it?

[johnpapa]

wow, thank you. i appreciated the conversation - but am issue or PR is even better. Thanks!

yes, feel free to create a new issue for this and let's hash it out.

BTW - I am getting peacock stickers. I'll send you a few if you shoot me your address (reach me via twitter DM)

[musicfuel]

I'll get the issue started later today.

Very cool about the stickers and I will get you my info. You should probably be the keeper of these anyway, but for the sticker effort (assuming you are using the logo) here are the source Illustrator file I used for the logo and a PDF of it in a *.zip. The vectors can scale up as needed for print.

Peacock-Logo.zip