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

add packages/stylelint-plugin & initial rule to prevent raw values #16924

Closed
wants to merge 2 commits into from

Conversation

senadir
Copy link
Contributor

@senadir senadir commented Aug 6, 2019

Description

after some initial talking with @jasmussen we had the idea to try stricter rules in the SCSS codebase.
This PR is a proposal toward a cleaner, less error-prone SCSS codebase.
it introduce a stylelint plugin for a basis to create new rules & extend existing rules.
the first rule it adds is declaration-strict-value and it prohibits the use of raw values in color,border-color,background,z-index, so things like this are not valid

.element {
    color: #e3e3e3;
}

.element {
    z-index: 1;
}

.element {
    background: #e3e3e3;
}

however, those are valid:

.element {
    color: theme(primary);
}
.element {
    color: transparent;
}
.element {
    color: currentColor;
}
.element {
    z-index: 0;
}

however, this only PR only add the detecting but doesn't fixes anything yet, there are 111 violation, not all of them are correct, but most of them are, if you want to ignore a violation you can turn off the rule.

/* stylelint-disable scale-unlimited/declaration-strict-value */
.has-pale-pink-background-color {
	background-color: #f78da7;
}
/* stylelint-enable */

what I would really appreciate is some help into fixing all of those issues, it's going to take some time to finish them all so feel free to push to this branch.

How has this been tested?

run npm install to install the new dependencies.

run npm run lint-css to see the new problems.

Screenshots

image

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@senadir senadir added [Status] In Progress Tracking issues with work in progress [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Code Quality Issues or PRs that relate to code quality npm Packages Related to npm packages labels Aug 6, 2019
@senadir senadir self-assigned this Aug 6, 2019
@senadir senadir changed the title add stylelint plugin & initial rule to prevent raw values add packages/stylelint-plugin & initial rule to prevent raw values Aug 6, 2019
@ntwb
Copy link
Member

ntwb commented Aug 6, 2019

Why not just add the stylelint stylelint-declaration-strict-value plugin to the existing stylelint-config-wordpress shared config?

For the most part it looks like these changes would be good for WP Core also.

@senadir
Copy link
Contributor Author

senadir commented Aug 6, 2019

my intention was to introduce more custom rules with time (things like accessibility checks, custom checks on complex values or checks that are logical for Gutenberg) that are ready to use plugin didn't provide.

do you think I should refactor this to be used directly inside the top level .stylelintrc.json and add a plugin later.

as for not adding it directly to stylelint-config-wordpress I actually haven't thought of that 😅

what do you suggest?

@youknowriad
Copy link
Contributor

I think it's the right opportunity to move stylelint-config-wordpress to the packages and consolidate under a unique package maintained with the rest of the WordPress packages.

@ntwb
Copy link
Member

ntwb commented Aug 6, 2019

Probably worth having a discussion in #core-js about this, might be a bit late for this weeks dev chat that is about to occur though.

For what its worth I also plan on moving stylelint-confit-wordpress to this repo and have it become @wordpress/stylelint-config, I’ve just not had the time to do that yet.

We can add a custom config to stylelint-config-wordpress, it already has two configs, the standard/default for CSS, and the additional for SCSS that extends the default CSS.

If needed we can add more rules and plugins such as stylelint-declaration-strict-value to the above SCSS config proving its acceptable for WP Core also, if not, it can be added here to the Gutenberg config.

If new stylelint plugins are to be created we should create a new package for each, eg, @wordpress/stylelint-plugin-abc, @wordpress/stylelint-plugin-xyz, etc so that they can also be used by the wider community

————

After rereading the above I think adding stylelint-declaration-strict-value to the existing stylelint config here in this repo would be best at this stage, then it can be added to stylelint-config-wordpress once WP has accepted that as a new Coding Standard.

P.s. stylelint is always spelled entirely in lowercase

@gziolo
Copy link
Member

gziolo commented Aug 27, 2019

For what its worth I also plan on moving stylelint-confit-wordpress to this repo and have it become @wordpress/stylelint-config, I’ve just not had the time to do that yet.

Yeah, it would be great to have everything colocated as we already have it for ESLint, Browserslist or Babel 👍

@gziolo
Copy link
Member

gziolo commented Jan 3, 2021

With #27810 landed – @wordpress/stylelint-config exists in the repository.

@gziolo gziolo added [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. and removed [Status] In Progress Tracking issues with work in progress labels Jan 3, 2021
@senadir
Copy link
Contributor Author

senadir commented Jan 4, 2021

so good to close this?

@gziolo
Copy link
Member

gziolo commented Jan 4, 2021

so good to close this?

I don't know whether it contains any of the changes proposed for enabled rules in this PR. The package itself is now maintained in the monorepo.

@ntwb
Copy link
Member

ntwb commented Jan 4, 2021

I think my previous comment above would still be somewhat related, we can add the stylelint-declaration-use-variable stylelint plugin to the now included stylelint package if agred upon to the config

This PR could be adapted for that, but maybe a new PR would be a little cleaner and easier, up to you @senadir

Base automatically changed from master to trunk March 1, 2021 15:42
@senadir senadir marked this pull request as ready for review August 11, 2021 10:16
@senadir senadir requested a review from ajitbohra as a code owner August 11, 2021 10:16
@kathrynwp
Copy link

Hi @senadir @gziolo - hey there! I'm circling back to this to see if it's still valid and planned, or whether it's best to close it out. Let me know, thank you!

@priethor
Copy link
Contributor

Let's close this issue and reopen it again if necessary. 🙏

@priethor priethor closed this Jun 30, 2023
@priethor priethor removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jun 30, 2023
@gziolo gziolo deleted the add/stylelint-plugin branch June 30, 2023 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants