10up / 10up-toolkit

Official 10up asset building toolkit.
113 stars 19 forks source link

Add support for CSS Custom Properties generated by theme.json #300

Closed ian-svoboda-prom closed 1 year ago

ian-svoboda-prom commented 1 year ago

Is your enhancement related to a problem? Please describe.

The current stylelint configuration has a rule that requires kebab-case variables. So if you attempt to use a CSS variable that is generated by WP (ex: theme.json color palette choices) it will throw an error because those variables use a double-hyphen (--) deliminater, such as: --wp--preset--color--white.

It seems clunky to have to create a secondary "alias variable" just to get around this, so for now I've just disabled the rule in the stylelint configuration.

Perhaps there's a way to set up the pattern matching to look for variables prefixed by --wp-- and allow those while enforcing the current pattern for custom ones? That might be a way to have our cake and eat it too.

Designs

No response

Describe alternatives you've considered

There are only a few options:

  1. Don't use the WP variables. This seems undesirable as those variables are coming from core.
  2. Add an ignore for the rule. This is easy, but it seems like something that wouldn't be obvious to someone less experienced with the tooling and/or WP.

Code of Conduct

Antonio-Laguna commented 1 year ago

Hey @ian-svoboda-prom !

I think this is not an issue anymore. This used to happen when using custom-property-pattern which is not included in our package, WordPress, stylelint-recommended or stylelint-recommended-scss.

Could you ensure your dependencies are up to date? I've checked on a couple of projects and though I've battled with this in the past, it doesn't seem to be brought up again by the linting nowadays.

Please let me know if you find something that could be fixed as I agree this would not be ideal

nicholasio commented 1 year ago

@ian-svoboda-prom Is this still an issue? Do you have a custom stylelint config?

JacksonCleary commented 1 year ago

Hello,

@nicholasio can confirm this is still an issue. Running into it right now for a project using v5.2.1.

@ian-svoboda-prom if possible can you share an example to bypass this if possible.

Antonio-Laguna commented 1 year ago

@JacksonCleary please can you provide mor information? We've been checking on some projects and couldn't replicate, would love to get to the bottom of this issue

iansvo commented 1 year ago

@Antonio-Laguna can confirm a warning is still thrown even with zero configuration and the latest toolkit version. The screenshot here shows the package.json file, my terminal output, and the code in question:

image

Basically it warns about kebab-case due to the custom-property-pattern rule, which you've said shouldn't actually be here. So that seems to suggest that something else is included that in their ruleset. You can see from this screenshot that I'm not doing things like customizing stylelint or the webpack config, it's a super barebones theme that I was creating just for the purpose of using 10up-toolkit (for a blog post).

I used node 18 when running the start command here, but I also replicated running node 16, so I doubt that's related.

Let me know if you have any other questions.

iansvo commented 1 year ago

@JacksonCleary The solution here is to add your own .stylelintrc config file to the theme root and then exclude a given rule, here's an example of what that file might look like:

{
    "extends": [
        "@10up/stylelint-config"
    ],
    "rules": {
        "custom-property-pattern": null,
    }
}

I hope that helps!

Antonio-Laguna commented 1 year ago

Thanks! Was able to find this! will prepare a PR soon

joesnellpdx commented 1 year ago

@Antonio-Laguna

I don't know if it being inherited somewhere else, but we still need to support BEM conventions:

block
block__element
block__element--modifier
block-test__element
block-test__element--modifier

This should work for all of them - I think (although I'm not an expert): ^(?:--\w+(?:[-_]\w+)*(?:--\w+(?:[-_]\w+)*)*|(?:\w+(?:__\w+)*(?:[-_]\w+)*(?:--\w+(?:__\w+)*(?:[-_]\w+)*)*)+)$

Antonio-Laguna commented 1 year ago

@joesnellpdx This change only affects custom property names and not selectors