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

style: Add CSS variables #241

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

style: Add CSS variables #241

wants to merge 1 commit into from

Conversation

Venefilyn
Copy link
Member

No description provided.

--tb-global-action--BorderColor: #B6C9DD;
--tb-global-header--BackgroundColor: #CEE3F8;

--tb-global-list--BackgroundColor: 247, 250, 253;
Copy link
Member Author

Choose a reason for hiding this comment

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

So I noticed some colors are used with alpha, if we do it like this instead of hex color or rgb(247, 250, 253) we can utilize rgb(var(--tb-global-list--BackgroundColor), 0.5)

Though that will become confusing with normal color variables unless we use different var naming

Copy link
Member

@eritbh eritbh Feb 29, 2020

Choose a reason for hiding this comment

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

I don't think there's a good way to handle this in raw CSS, though I'm not sure we have so many opacity variations that we need to. We can always just have two variables, --tb-whatever-color and --tb-whatever-color-transparent, or something like that.

I wish the opacify() Sass function were useful here, but since it's part of the preprocessor, it's useless with pure CSS variables. We'd have to define all our colors as Sass variables, then define the CSS variables in terms of the Sass variables and opacify().

@Venefilyn Venefilyn changed the title style: experiment with CSS vars style: Add CSS variables Feb 29, 2020
@eritbh
Copy link
Member

eritbh commented Feb 29, 2020

I have a feeling there's gonna be a fair amount of bikeshedding about naming conventions in this PR...

I'm not a fan of naming conventions that mix names-like-this and NamesLikeThis. I vastly prefer HTML/CSS to use all-lowercase, hyphenated names for classes, IDs, etc., and naming variables the same way just makes more sense to me. I'd also like to be linked somewhere that explains the benefits to using double--separators and sometimes different--double__separators as a naming convention for CSS. I'm sure it has a purpose, but I've avoided it like the plague in past projects because it just looks inconsistent and less readable to me.

@Venefilyn
Copy link
Member Author

I've been following how PatternFly styling did it somewhat. It follows this:

--pf-c-block[__element][--modifier][--state][--breakpoint][--pseudo-element]--PropertyCamelCase

@creesch
Copy link
Member

creesch commented Feb 29, 2020

I have to agree with @Geo1088 here in regards to mixing different naming conventions.

So I'd rather see something along the lines of --tb-global-header-background-color or if we want to differentiate between the first and latter part maybe use an underscore like so --tb-global-header_background-color

@eritbh
Copy link
Member

eritbh commented Feb 29, 2020

Yeah I don't really think that naming convention applies super well to this project. For bigger codebases with lots more variability, it makes sense to have lots of options for stuff like sizing modifiers, global vs. component styles, and all that, but Toolbox just doesn't have that much to change. There's really not even that many places in our code where we use helper classes or anything like that, because we're building interface components that will only ever be used inside our own codebase. So following a naming convention that has strict specifications on how you encode all the various modifiers and stuff seems overkill to me.

I'd much rather base variable names on our existing "conventions" on class naming. If we've got a .tb-window-header that needs a background, why not just call it --tb-window-header-background? There are plenty of places in the code where our class names are bad or inconsistent, and I know stricter naming conventions can have benefits, but I'm not super convinced that making our variable names follow a stricter convention is actually going to help a whole lot because of our scale and such.

@k3n
Copy link
Contributor

k3n commented Feb 29, 2020

IMHO it's not bikeshedding if it's productive and not weaponized (whether deliberately or not). I think coming to a consensus on a naming scheme is important.

+1 to @SpyTec for getting the ball rolling!
+1 for lowercase
+1 for kebab-case (had to look that up)
+1 for a single underscore to delineate names from ancillary data like CSS property or DOM state

@eritbh
Copy link
Member

eritbh commented Feb 29, 2020

+1 for a single underscore to delineate names from ancillary data like CSS property or DOM state

I'm kinda against this, to be honest. I'd rather our variables not be tied to specific properties or states; rather, I'd prefer to have a set of variables that just define certain colors/widths/etc that can be used in multiple situations, so CSS properties and states can be defined on their own without needing the variable to match.

For example, instead of:

:root {
	--tb-header_background-color: #abcdef;
}
.tb-window-header {
	background-color: var(--tb-header-background-color);
}

I'd rather it look something like this:

:root {
	--tb-blue-light: #abcdef;
}
.tb-window-header {
	background-color: var(--tb-blue-light);
}

I'm sure better names could be used for the various colors we have already, but you get the idea - have variable names describe the values they hold rather than the places they're referenced.

@Venefilyn
Copy link
Member Author

I'm sure better names could be used for the various colors we have already, but you get the idea - have variable names describe the values they hold rather than the places they're referenced.

This will work for most things we do, which is great. But there may still be areas of the code if not now then eventually that will be module specific.

If we do go for a --tb- prefix with kebab-case where naming is describing the usage of the color (primary, primary-bg, secondary, secondary-pg, primary-border, etc.) that would make things easier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants