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 new package @wordpress/stylelint config #22777

Merged
merged 355 commits into from
Dec 18, 2020
Merged

Conversation

ntwb
Copy link
Member

@ntwb ntwb commented May 31, 2020

Description

Migrates the WordPress-Coding-Standards/stylelint-config-wordpress into this repo as a new @wordpress/stylelint-config package using lerna import

Import Repo

Using Lerna the existing git repo was imported using:

lerna import ../stylelint-config-wordpress/ --flatten
lerna info About to import 320 commits from ../stylelint-config-wordpress/ into packages/stylelint-config-wordpress

Note: As such this PR includes a lot of commits, the oldest 320 commits are from the legacy repo

Testing

To test run the following standalone tests:

  1. Change to the repo root
  2. Remove node_modules, rm -rf node_modules
  3. Run npm install
  4. Run npm run build
  5. Run npm run lint-css from the repo root
  6. Run npm run test-unit from the repo root

ToDo

  • The legacy code is licensed under MIT, can discuss switching to GPL
  • Update the package author field
  • Update readme
  • Update changelog
  • Rewrite changelog in packages markdown format
  • Add wp-scripts format-css

Update packages per /packages#managing-dependencies

  • Update root packages
  • Update @wordpress/scripts packages

How has this been tested?

Screenshots

Types of changes

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

ntwb and others added 30 commits May 31, 2020 15:33
* feat: Add SCSS preset config

* fix: Include README.md and scss.js in package.json files list

* test: Add initial SCSS tests
… a promise and move css code out of tests to individual files

* feat: Use ECMAScript 8/2017

* refactor: Use async/await instead of returning a promise and move css code out of tests to individual files
* chore(package): update ava to version 0.17.0
* refactor: Include path to test fixtures

https://greenkeeper.io/
* chore(package): update eslint-config-stylelint to version 6.0.0
* refactor: Update tests per latest `eslint-config-stylelint`
…118)

* chore: Drop NodeJS from Travis CI

* chore: Drop NodeJS from AppVeyor

* chore: Bump minimum NodeJS requirement to 6.9.1
@gziolo
Copy link
Member

gziolo commented Dec 15, 2020

@ntwb, would be best to answer the version question. In the past, we migrated other packages and applied the major version bump. I don't mind following the same approach here.

@ocean90
Copy link
Member

ocean90 commented Dec 15, 2020

and applied the major version bump. I don't mind following the same approach here.

So that would be 18.0.0? That works for me too. This would also allow us to reduce the version constraints in peerDependencies so we can publish this with the fix from WordPress-Coding-Standards/stylelint-config-wordpress#267.

@ocean90 ocean90 marked this pull request as ready for review December 15, 2020 21:02
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Overall, it looks like it's ready to merge. It's hard to tell if functionality wise everything is correct as I have no idea what the package contains as of today. If @ntwb or anyone else maintaining is happy then I'm happy as well 😃

packages/stylelint-config/package.json Show resolved Hide resolved
@@ -0,0 +1,22 @@
The MIT License (MIT)
Copy link
Member

Choose a reason for hiding this comment

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

Is this license file necessary? It looks like a standard one. I have no idea myself, just asking. We don't put copies for "GPL-2.0-or-later" in other packages.

Copy link
Member

Choose a reason for hiding this comment

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

Good question, I'm not sure either. Wondering who would be best to answer this?

At least the license itself says, that it should be included. But the copyright info is actually a bit odd. 🤷🏻‍♂️

@gziolo
Copy link
Member

gziolo commented Dec 16, 2020

I left a few more nitpicks among which I care mostly about the version bump one 😄 Once it's addressed I'm happy to approve this PR based on my limited knowledge as raised in the previous comment.

@gziolo
Copy link
Member

gziolo commented Dec 17, 2020

I have just published WordPress packages to npm. When running wp-scripts lint-style I see the following error:

Deprecation Warning: 'declaration-property-unit-whitelist' has been deprecated. Instead use 'declaration-property-unit-allowed-list'. See: https://github.com/stylelint/stylelint/blob/13.7.0/lib/rules/declaration-property-unit-whitelist/README.md

Is it covered in this PR? If not, I opened an issue: #27775.

@ocean90
Copy link
Member

ocean90 commented Dec 17, 2020

@gziolo Yes and no, referencing #22777 (comment) again.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Once CI is green this PR should be good to go. Some e2e tests became unstable but changes proposed here should not have impact on them at all. You might need to restart them when they fail.

@ocean90
Copy link
Member

ocean90 commented Dec 18, 2020

@gziolo I can't get End-to-End Tests / Admin - 3 (pull_request) passing, I guess that's ok for now?

@gziolo gziolo merged commit 9367df0 into master Dec 18, 2020
@gziolo gziolo deleted the try/stylelint-config-package branch December 18, 2020 08:28
@github-actions github-actions bot added this to the Gutenberg 9.7 milestone Dec 18, 2020
@gziolo
Copy link
Member

gziolo commented Dec 18, 2020

Yes, it's failing on the majority CI checks now 🤷‍♂️

I merged PR regardless 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] stylelint config /packages/stylelint-config
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants