Colors moved to variables in SCSS

[LorincJuraj]

This will simplify theming process and integration into existing project.

[tjschuck]

/cc @mlettini

[mlettini]

I'm not a fan of using two different colors here to represent a gradient. You have gradients in variables below, so why not do the same here. Also, we could clarify that these are background colors. I'd recommend:

$chosen-highlighted-bg-color: #3875d7;
$chosen-highlighted-bg-gradient: linear-gradient(#3875d7 20%, #2a62bc 90%);

[mlettini]

There's a lot of colors here. I like what you started with clarifying whether a color is background or border or box-shadow or whatever. I think we should keep that going with text colors too, so I'd recommend this be $chosen-group-name-text-color.

[mlettini]

Same "text" comment here: $chosen-search-choice-text-color

[mlettini]

I'd recommend us using the word "gradient" here, and in terms of line ordering, this should be bundled with $chosen-search-choice-bg-color, since those two are next to each other in the css too.

$chosen-search-choice-bg-color: #eee;
$chosen-search-choice-bg-gradient: linear-gradient(#f4f4f4 20%, #f0f0f0 50%, #e8e8e8 52%, #eee 100%);

[mlettini]

Same "text" comment here: $chosen-search-choice-disabled-text-color

[mlettini]

For consistency with other variable names, this should be $chosen-no-results-bg-color

[mlettini]

Same "text" comment here: $chosen-text-color

[mlettini]

This is the first actual issue I see in the code. $chosen-border-color is already being defined above. Looking at the CSS, #5897fb is only used when active/focused. So I think this should be $chosen-active-border-color. I see some other active/focused-specific variables, and I think those variables should be grouped together, and have consistent naming.

[mlettini]

Same "text" comment here: $chosen-single-text-color

[mlettini]

I'm not sure how I feel about using a variable inside the definition of another variable. Normally I'd be all for it, but in this case (and a few other cases I see in this file), the variables are completely unrelated. For instance, $chosen-drop-result-selected has absolutely nothing to do with $chosen-search-choice-disabled-border-color (other than being a similar hex color), and by making the former defined by the latter, the code is making the two seem related in some way. I'll let another dev make a call on that (cc @stof)

[mlettini]

I love this idea of moving colors into variables to prepare for better theming. Nice work @LorincJuraj.

I didn't actually finish leaving all the comments I had, because I realized 1. I was adding those comments individually instead of all at once with a review, which can be annoying for notifications (sorry didn't mean to do that), and 2. I was repeating myself regarding naming conventions. Most of my comments are either about naming, or which variables we group together. So my feedback can be boiled down to essentially:

• Let's keep the naming consistent and visual. If the color is for a background color, let's use -bg-color:, for background gradient -bg-gradient, for text color -text-color, etc etc.
• Let's keep the order and grouping of variables consistent with how they are used in the CSS below. So all -bg-color:s and -bg-gradients: should be next to each other, all active/focus colors grouped together, disabled, single, multi, etc etc.

So take that into account with all the variables, including the lines I didn't comment on. ✌️

[LorincJuraj]

@mlettini Thanks for comments. You are right I should be more consistent with the naming. I was doing this quickly when I was testing Chosen on one of my projects and I needed quick working solution. If you have the variables renamed feel free to do the commit, because I am not sure, when will I get a chance to do it myself, because i have some deadlines which are getting pretty tight, so I have to focus on them right now.