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

Create a Nested version of the css-vars mixin #2

Closed
wants to merge 3 commits into from

Conversation

RedLucas
Copy link

@RedLucas RedLucas commented Jun 6, 2017

The idea of this is to allow you to apply variables in a nested context. So you can apply variables to a class, and not global. I couldn't think of a way to detect if we are root already so I made a second function and tried to take out the stuff from both and put it into its own function.

Let me know what you think, but I think this is pretty cool.

RedLucas and others added 3 commits June 5, 2017 10:26
Allow the css vars to be natively namespaced.
@@ -1,6 +1,6 @@
{
"name": "css-var",
"version": "2.1.1",
"name": "css-vars",
Copy link
Owner

Choose a reason for hiding this comment

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

The name is already taken in bower, so it was not a typo, it was on purpose

Copy link
Author

Choose a reason for hiding this comment

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

Ah, sorry, didn't know that.

@shospodarets
Copy link
Owner

Hey @RedLucas
Thank you for the PR.
I provided some comments.
Thank you for the idea, in general it's nice, just breaks the idea of having all the variables globally, to avoid problems.
Take a look into the postcss-css-variables discussions, why they store everything on the :root scope:
postcss/postcss-custom-properties#1
postcss/postcss-custom-properties#9

That's why I'm not sure this addition will not provide additional unexpected behavior.

@RedLucas
Copy link
Author

I also get the concern regarding unexpected behavior, though I am using this and so far I've been happy with it.

I need it to work like this for a project I'm working on, though I could always just use my fork... I'd prefer to contribute it back into this project if that's possible.

Just need to keep using this thing until Microsoft drops support for IE11, then maybe we can ascend to a greater state of being.

///
@mixin css-vars($varMap: null) {
// CHECK PARAMS
@mixin _css-vars($varMap: null) {
Copy link
Owner

Choose a reason for hiding this comment

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

In this case better to exclude the CHECK_PARAMS part and call the new mixin differently to reflect it

// ));
///
@mixin css-vars-nested($varMap: null) {
Copy link
Owner

Choose a reason for hiding this comment

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

The mixins css-vars and and css-vars-nested are the same,
except the parts "@at-root :root {" and "& {" - please consider extending that to follow the DRY

Copy link
Owner

Choose a reason for hiding this comment

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

Can you please update the mixins comments to reflect the difference between them

Copy link
Owner

Choose a reason for hiding this comment

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

And the last one- with such an update README should be changed with the provided addition

// ));
///
@mixin css-vars-nested($varMap: null) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you consider adding tests for this as well

@shospodarets
Copy link
Owner

Closing as the PR is not mergable and not updated for a year.

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